* [PATCH 01/11] OvmfPkg/XenBusDxe: Fix missing \n in DEBUG messages
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 14:24 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 02/11] OvmfPkg/XenBusDxe: Have XenStoreFindWatch take a pointer Anthony PERARD
` (9 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
Fix missing \n in DEBUG messages in XenBusDxe and use DEBUG_*.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/EventChannel.c | 3 ++-
OvmfPkg/XenBusDxe/XenStore.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
index 6900071782..c6b3871781 100644
--- a/OvmfPkg/XenBusDxe/EventChannel.c
+++ b/OvmfPkg/XenBusDxe/EventChannel.c
@@ -44,7 +44,8 @@ XenBusEventChannelAllocate (
EVTCHNOP_alloc_unbound,
&Parameter);
if (ReturnCode != 0) {
- DEBUG ((EFI_D_ERROR, "ERROR: alloc_unbound failed with rc=%d", ReturnCode));
+ DEBUG ((DEBUG_ERROR, "ERROR: alloc_unbound failed with rc=%d\n",
+ ReturnCode));
return ReturnCode;
}
*Port = Parameter.port;
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 34890ae40b..7253d8ae37 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -738,7 +738,7 @@ XenStoreReadReply (
XENSTORE_STATUS Status;
Status = XenStoreProcessMessage ();
if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
- DEBUG ((EFI_D_ERROR, "XenStore, error while reading the ring (%d).",
+ DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
Status));
return Status;
}
@@ -1076,7 +1076,7 @@ XenStoreDeinit (
if (!IsListEmpty (&xs.RegisteredWatches)) {
XENSTORE_WATCH *Watch;
LIST_ENTRY *Entry;
- DEBUG ((EFI_D_WARN, "XenStore: RegisteredWatches is not empty, cleaning up..."));
+ DEBUG ((DEBUG_WARN, "XenStore: RegisteredWatches is not empty, cleaning up...\n"));
Entry = GetFirstNode (&xs.RegisteredWatches);
while (!IsNull (&xs.RegisteredWatches, Entry)) {
Watch = XENSTORE_WATCH_FROM_LINK (Entry);
@@ -1092,7 +1092,7 @@ XenStoreDeinit (
//
if (!IsListEmpty (&xs.WatchEvents)) {
LIST_ENTRY *Entry;
- DEBUG ((EFI_D_WARN, "XenStore: WatchEvents is not empty, cleaning up..."));
+ DEBUG ((DEBUG_WARN, "XenStore: WatchEvents is not empty, cleaning up...\n"));
Entry = GetFirstNode (&xs.WatchEvents);
while (!IsNull (&xs.WatchEvents, Entry)) {
XENSTORE_MESSAGE *Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 01/11] OvmfPkg/XenBusDxe: Fix missing \n in DEBUG messages
2019-09-13 14:50 ` [PATCH 01/11] OvmfPkg/XenBusDxe: Fix missing \n in DEBUG messages Anthony PERARD
@ 2019-09-16 14:24 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 14:24 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> Fix missing \n in DEBUG messages in XenBusDxe and use DEBUG_*.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/EventChannel.c | 3 ++-
> OvmfPkg/XenBusDxe/XenStore.c | 6 +++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
> index 6900071782..c6b3871781 100644
> --- a/OvmfPkg/XenBusDxe/EventChannel.c
> +++ b/OvmfPkg/XenBusDxe/EventChannel.c
> @@ -44,7 +44,8 @@ XenBusEventChannelAllocate (
> EVTCHNOP_alloc_unbound,
> &Parameter);
> if (ReturnCode != 0) {
> - DEBUG ((EFI_D_ERROR, "ERROR: alloc_unbound failed with rc=%d", ReturnCode));
> + DEBUG ((DEBUG_ERROR, "ERROR: alloc_unbound failed with rc=%d\n",
> + ReturnCode));
> return ReturnCode;
> }
> *Port = Parameter.port;
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 34890ae40b..7253d8ae37 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -738,7 +738,7 @@ XenStoreReadReply (
> XENSTORE_STATUS Status;
> Status = XenStoreProcessMessage ();
> if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
> - DEBUG ((EFI_D_ERROR, "XenStore, error while reading the ring (%d).",
> + DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
> Status));
> return Status;
> }
> @@ -1076,7 +1076,7 @@ XenStoreDeinit (
> if (!IsListEmpty (&xs.RegisteredWatches)) {
> XENSTORE_WATCH *Watch;
> LIST_ENTRY *Entry;
> - DEBUG ((EFI_D_WARN, "XenStore: RegisteredWatches is not empty, cleaning up..."));
> + DEBUG ((DEBUG_WARN, "XenStore: RegisteredWatches is not empty, cleaning up...\n"));
> Entry = GetFirstNode (&xs.RegisteredWatches);
> while (!IsNull (&xs.RegisteredWatches, Entry)) {
> Watch = XENSTORE_WATCH_FROM_LINK (Entry);
> @@ -1092,7 +1092,7 @@ XenStoreDeinit (
> //
> if (!IsListEmpty (&xs.WatchEvents)) {
> LIST_ENTRY *Entry;
> - DEBUG ((EFI_D_WARN, "XenStore: WatchEvents is not empty, cleaning up..."));
> + DEBUG ((DEBUG_WARN, "XenStore: WatchEvents is not empty, cleaning up...\n"));
> Entry = GetFirstNode (&xs.WatchEvents);
> while (!IsNull (&xs.WatchEvents, Entry)) {
> XENSTORE_MESSAGE *Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 02/11] OvmfPkg/XenBusDxe: Have XenStoreFindWatch take a pointer
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
2019-09-13 14:50 ` [PATCH 01/11] OvmfPkg/XenBusDxe: Fix missing \n in DEBUG messages Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 14:38 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 03/11] OvmfPkg/XenBusDxe: Rework watch events reception Anthony PERARD
` (8 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
Rework XenStoreFindWatch() to be able to search for a registered watch
with a pointer instead of a string.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenStore.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 7253d8ae37..727641a0fe 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -253,14 +253,12 @@ Split (
STATIC
XENSTORE_WATCH *
XenStoreFindWatch (
- IN CONST CHAR8 *Token
+ IN VOID *Token
)
{
- XENSTORE_WATCH *Watch, *WantedWatch;
+ XENSTORE_WATCH *Watch;
LIST_ENTRY *Entry;
- WantedWatch = (VOID *) AsciiStrHexToUintn (Token);
-
if (IsListEmpty (&xs.RegisteredWatches)) {
return NULL;
}
@@ -268,7 +266,7 @@ XenStoreFindWatch (
!IsNull (&xs.RegisteredWatches, Entry);
Entry = GetNextNode (&xs.RegisteredWatches, Entry)) {
Watch = XENSTORE_WATCH_FROM_LINK (Entry);
- if (Watch == WantedWatch)
+ if ((VOID *) Watch == Token)
return Watch;
}
@@ -632,12 +630,16 @@ XenStoreProcessMessage (
Body[Message->Header.len] = '\0';
if (Message->Header.type == XS_WATCH_EVENT) {
+ VOID *ConvertedToken;
+
Message->u.Watch.Vector = Split(Body, Message->Header.len,
&Message->u.Watch.VectorSize);
+ ConvertedToken =
+ (VOID *) AsciiStrHexToUintn (Message->u.Watch.Vector[XS_WATCH_TOKEN]);
+
EfiAcquireLock (&xs.RegisteredWatchesLock);
- Message->u.Watch.Handle =
- XenStoreFindWatch (Message->u.Watch.Vector[XS_WATCH_TOKEN]);
+ Message->u.Watch.Handle = XenStoreFindWatch (ConvertedToken);
DEBUG ((EFI_D_INFO, "XenStore: Watch event %a\n",
Message->u.Watch.Vector[XS_WATCH_TOKEN]));
if (Message->u.Watch.Handle != NULL) {
@@ -1384,8 +1386,7 @@ XenStoreUnregisterWatch (
ASSERT (Watch->Signature == XENSTORE_WATCH_SIGNATURE);
- AsciiSPrint (Token, sizeof (Token), "%p", (VOID *) Watch);
- if (XenStoreFindWatch (Token) == NULL) {
+ if (XenStoreFindWatch (Watch) == NULL) {
return;
}
@@ -1393,6 +1394,7 @@ XenStoreUnregisterWatch (
RemoveEntryList (&Watch->Link);
EfiReleaseLock (&xs.RegisteredWatchesLock);
+ AsciiSPrint (Token, sizeof (Token), "%p", (VOID *) Watch);
XenStoreUnwatch (Watch->Node, Token);
/* Cancel pending watch events. */
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 02/11] OvmfPkg/XenBusDxe: Have XenStoreFindWatch take a pointer
2019-09-13 14:50 ` [PATCH 02/11] OvmfPkg/XenBusDxe: Have XenStoreFindWatch take a pointer Anthony PERARD
@ 2019-09-16 14:38 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 14:38 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> Rework XenStoreFindWatch() to be able to search for a registered watch
> with a pointer instead of a string.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/XenStore.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 7253d8ae37..727641a0fe 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -253,14 +253,12 @@ Split (
> STATIC
> XENSTORE_WATCH *
> XenStoreFindWatch (
> - IN CONST CHAR8 *Token
> + IN VOID *Token
> )
> {
> - XENSTORE_WATCH *Watch, *WantedWatch;
> + XENSTORE_WATCH *Watch;
> LIST_ENTRY *Entry;
>
> - WantedWatch = (VOID *) AsciiStrHexToUintn (Token);
> -
> if (IsListEmpty (&xs.RegisteredWatches)) {
> return NULL;
> }
> @@ -268,7 +266,7 @@ XenStoreFindWatch (
> !IsNull (&xs.RegisteredWatches, Entry);
> Entry = GetNextNode (&xs.RegisteredWatches, Entry)) {
> Watch = XENSTORE_WATCH_FROM_LINK (Entry);
> - if (Watch == WantedWatch)
> + if ((VOID *) Watch == Token)
> return Watch;
> }
>
> @@ -632,12 +630,16 @@ XenStoreProcessMessage (
> Body[Message->Header.len] = '\0';
>
> if (Message->Header.type == XS_WATCH_EVENT) {
> + VOID *ConvertedToken;
> +
> Message->u.Watch.Vector = Split(Body, Message->Header.len,
> &Message->u.Watch.VectorSize);
>
> + ConvertedToken =
> + (VOID *) AsciiStrHexToUintn (Message->u.Watch.Vector[XS_WATCH_TOKEN]);
> +
> EfiAcquireLock (&xs.RegisteredWatchesLock);
> - Message->u.Watch.Handle =
> - XenStoreFindWatch (Message->u.Watch.Vector[XS_WATCH_TOKEN]);
> + Message->u.Watch.Handle = XenStoreFindWatch (ConvertedToken);
> DEBUG ((EFI_D_INFO, "XenStore: Watch event %a\n",
> Message->u.Watch.Vector[XS_WATCH_TOKEN]));
> if (Message->u.Watch.Handle != NULL) {
> @@ -1384,8 +1386,7 @@ XenStoreUnregisterWatch (
>
> ASSERT (Watch->Signature == XENSTORE_WATCH_SIGNATURE);
>
> - AsciiSPrint (Token, sizeof (Token), "%p", (VOID *) Watch);
> - if (XenStoreFindWatch (Token) == NULL) {
> + if (XenStoreFindWatch (Watch) == NULL) {
> return;
> }
>
> @@ -1393,6 +1394,7 @@ XenStoreUnregisterWatch (
> RemoveEntryList (&Watch->Link);
> EfiReleaseLock (&xs.RegisteredWatchesLock);
>
> + AsciiSPrint (Token, sizeof (Token), "%p", (VOID *) Watch);
> XenStoreUnwatch (Watch->Node, Token);
>
> /* Cancel pending watch events. */
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 03/11] OvmfPkg/XenBusDxe: Rework watch events reception
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
2019-09-13 14:50 ` [PATCH 01/11] OvmfPkg/XenBusDxe: Fix missing \n in DEBUG messages Anthony PERARD
2019-09-13 14:50 ` [PATCH 02/11] OvmfPkg/XenBusDxe: Have XenStoreFindWatch take a pointer Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 14:39 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 04/11] OvmfPkg/XenBusDxe: Avoid Allocate in XenStoreVSPrint Anthony PERARD
` (7 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
This patch rework the reception of xenstore watch event to avoid
allocation.
Instead of queuing watch events, we simply mark a XENSTORE_WATCH as
"triggered". We don't need to know how many time we received the
event, only that it happened. That avoid to allocate a
XENSTORE_MESSAGE for every watch events.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenStore.c | 125 ++++++++++-------------------------
1 file changed, 35 insertions(+), 90 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 727641a0fe..5cc900190a 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -65,6 +65,8 @@ struct _XENSTORE_WATCH
/* Path being watched. */
CHAR8 *Node;
+
+ BOOLEAN Triggered;
};
#define XENSTORE_WATCH_FROM_LINK(l) \
@@ -86,13 +88,6 @@ typedef struct {
struct {
CHAR8 *Body;
} Reply;
-
- /* Queued watch events. */
- struct {
- XENSTORE_WATCH *Handle;
- CONST CHAR8 **Vector;
- UINT32 VectorSize;
- } Watch;
} u;
} XENSTORE_MESSAGE;
#define XENSTORE_MESSAGE_FROM_LINK(r) \
@@ -133,14 +128,6 @@ typedef struct {
/** Lock protecting the registered watches list. */
EFI_LOCK RegisteredWatchesLock;
- /**
- * List of pending watch callback events.
- */
- LIST_ENTRY WatchEvents;
-
- /** Lock protecting the watch calback list. */
- EFI_LOCK WatchEventsLock;
-
/**
* The event channel for communicating with the
* XenStore service.
@@ -630,29 +617,32 @@ XenStoreProcessMessage (
Body[Message->Header.len] = '\0';
if (Message->Header.type == XS_WATCH_EVENT) {
- VOID *ConvertedToken;
+ CONST CHAR8 *WatchEventPath;
+ CONST CHAR8 *WatchEventToken;
+ VOID *ConvertedToken;
+ XENSTORE_WATCH *Watch;
- Message->u.Watch.Vector = Split(Body, Message->Header.len,
- &Message->u.Watch.VectorSize);
+ //
+ // Parse WATCH_EVENT messages
+ // <path>\0<token>\0
+ //
+ WatchEventPath = Body;
+ WatchEventToken = WatchEventPath + AsciiStrSize (WatchEventPath);
- ConvertedToken =
- (VOID *) AsciiStrHexToUintn (Message->u.Watch.Vector[XS_WATCH_TOKEN]);
+ ConvertedToken = (VOID *) AsciiStrHexToUintn (WatchEventToken);
EfiAcquireLock (&xs.RegisteredWatchesLock);
- Message->u.Watch.Handle = XenStoreFindWatch (ConvertedToken);
- DEBUG ((EFI_D_INFO, "XenStore: Watch event %a\n",
- Message->u.Watch.Vector[XS_WATCH_TOKEN]));
- if (Message->u.Watch.Handle != NULL) {
- EfiAcquireLock (&xs.WatchEventsLock);
- InsertHeadList (&xs.WatchEvents, &Message->Link);
- EfiReleaseLock (&xs.WatchEventsLock);
+ Watch = XenStoreFindWatch (ConvertedToken);
+ DEBUG ((DEBUG_INFO, "XenStore: Watch event %a\n", WatchEventToken));
+ if (Watch != NULL) {
+ Watch->Triggered = TRUE;
} else {
DEBUG ((EFI_D_WARN, "XenStore: Watch handle %a not found\n",
- Message->u.Watch.Vector[XS_WATCH_TOKEN]));
- FreePool((VOID*)Message->u.Watch.Vector);
- FreePool(Message);
+ WatchEventToken));
}
EfiReleaseLock (&xs.RegisteredWatchesLock);
+ FreePool (Message);
+ FreePool (Body);
} else {
Message->u.Reply.Body = Body;
EfiAcquireLock (&xs.ReplyLock);
@@ -936,40 +926,29 @@ XenStoreUnwatch (
STATIC
XENSTORE_STATUS
XenStoreWaitWatch (
- VOID *Token
+ IN VOID *Token
)
{
- XENSTORE_MESSAGE *Message;
- LIST_ENTRY *Entry = NULL;
- LIST_ENTRY *Last = NULL;
+ XENSTORE_WATCH *Watch;
XENSTORE_STATUS Status;
+ EfiAcquireLock (&xs.RegisteredWatchesLock);
+ Watch = XenStoreFindWatch (Token);
+ EfiReleaseLock (&xs.RegisteredWatchesLock);
+ if (Watch == NULL) {
+ return XENSTORE_STATUS_EINVAL;
+ }
+
while (TRUE) {
- EfiAcquireLock (&xs.WatchEventsLock);
- if (IsListEmpty (&xs.WatchEvents) ||
- Last == GetFirstNode (&xs.WatchEvents)) {
- EfiReleaseLock (&xs.WatchEventsLock);
- Status = XenStoreProcessMessage ();
- if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
- return Status;
- }
- continue;
+ if (Watch->Triggered) {
+ Watch->Triggered = FALSE;
+ return XENSTORE_STATUS_SUCCESS;
}
- for (Entry = GetFirstNode (&xs.WatchEvents);
- Entry != Last && !IsNull (&xs.WatchEvents, Entry);
- Entry = GetNextNode (&xs.WatchEvents, Entry)) {
- Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
- if (Message->u.Watch.Handle == Token) {
- RemoveEntryList (Entry);
- EfiReleaseLock (&xs.WatchEventsLock);
- FreePool((VOID*)Message->u.Watch.Vector);
- FreePool(Message);
- return XENSTORE_STATUS_SUCCESS;
- }
+ Status = XenStoreProcessMessage ();
+ if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
+ return Status;
}
- Last = GetFirstNode (&xs.WatchEvents);
- EfiReleaseLock (&xs.WatchEventsLock);
}
}
@@ -1052,12 +1031,10 @@ XenStoreInit (
xs.XenStore, xs.EventChannel));
InitializeListHead (&xs.ReplyList);
- InitializeListHead (&xs.WatchEvents);
InitializeListHead (&xs.RegisteredWatches);
EfiInitializeLock (&xs.ReplyLock, TPL_NOTIFY);
EfiInitializeLock (&xs.RegisteredWatchesLock, TPL_NOTIFY);
- EfiInitializeLock (&xs.WatchEventsLock, TPL_NOTIFY);
/* Initialize the shared memory rings to talk to xenstored */
Status = XenStoreInitComms (&xs);
@@ -1088,23 +1065,6 @@ XenStoreDeinit (
}
}
- //
- // Emptying the list WatchEvents, but this list should already be empty after
- // having cleanup the list RegisteredWatches.
- //
- if (!IsListEmpty (&xs.WatchEvents)) {
- LIST_ENTRY *Entry;
- DEBUG ((DEBUG_WARN, "XenStore: WatchEvents is not empty, cleaning up...\n"));
- Entry = GetFirstNode (&xs.WatchEvents);
- while (!IsNull (&xs.WatchEvents, Entry)) {
- XENSTORE_MESSAGE *Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
- Entry = GetNextNode (&xs.WatchEvents, Entry);
- RemoveEntryList (&Message->Link);
- FreePool ((VOID*)Message->u.Watch.Vector);
- FreePool (Message);
- }
- }
-
if (!IsListEmpty (&xs.ReplyList)) {
XENSTORE_MESSAGE *Message;
LIST_ENTRY *Entry;
@@ -1382,7 +1342,6 @@ XenStoreUnregisterWatch (
)
{
CHAR8 Token[sizeof (Watch) * 2 + 1];
- LIST_ENTRY *Entry;
ASSERT (Watch->Signature == XENSTORE_WATCH_SIGNATURE);
@@ -1397,20 +1356,6 @@ XenStoreUnregisterWatch (
AsciiSPrint (Token, sizeof (Token), "%p", (VOID *) Watch);
XenStoreUnwatch (Watch->Node, Token);
- /* Cancel pending watch events. */
- EfiAcquireLock (&xs.WatchEventsLock);
- Entry = GetFirstNode (&xs.WatchEvents);
- while (!IsNull (&xs.WatchEvents, Entry)) {
- XENSTORE_MESSAGE *Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
- Entry = GetNextNode (&xs.WatchEvents, Entry);
- if (Message->u.Watch.Handle == Watch) {
- RemoveEntryList (&Message->Link);
- FreePool ((VOID*)Message->u.Watch.Vector);
- FreePool (Message);
- }
- }
- EfiReleaseLock (&xs.WatchEventsLock);
-
FreePool (Watch->Node);
FreePool (Watch);
}
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 03/11] OvmfPkg/XenBusDxe: Rework watch events reception
2019-09-13 14:50 ` [PATCH 03/11] OvmfPkg/XenBusDxe: Rework watch events reception Anthony PERARD
@ 2019-09-16 14:39 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 14:39 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> This patch rework the reception of xenstore watch event to avoid
> allocation.
>
> Instead of queuing watch events, we simply mark a XENSTORE_WATCH as
> "triggered". We don't need to know how many time we received the
> event, only that it happened. That avoid to allocate a
> XENSTORE_MESSAGE for every watch events.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/XenStore.c | 125 ++++++++++-------------------------
> 1 file changed, 35 insertions(+), 90 deletions(-)
This looks a bit more complex than what I can allocate time for now, so
I'll trust you on it -- it only modifies XenStore.c. Feedback from other
reviewers is encouraged.
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 04/11] OvmfPkg/XenBusDxe: Avoid Allocate in XenStoreVSPrint
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (2 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 03/11] OvmfPkg/XenBusDxe: Rework watch events reception Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 14:45 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation Anthony PERARD
` (6 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
In order to be able to use XenStoreVSPrint during the
ExitBootServices, we remove the allocation done by the function and
use the stack instead.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenStore.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 5cc900190a..7b71dc156d 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1259,20 +1259,17 @@ XenStoreVSPrint (
IN VA_LIST Marker
)
{
- CHAR8 *Buf;
- XENSTORE_STATUS Status;
- UINTN BufSize;
- VA_LIST Marker2;
+ CHAR8 Buf[XENSTORE_PAYLOAD_MAX];
+ UINTN Count;
- VA_COPY (Marker2, Marker);
- BufSize = SPrintLengthAsciiFormat (FormatString, Marker2) + 1;
- VA_END (Marker2);
- Buf = AllocateZeroPool (BufSize);
- AsciiVSPrint (Buf, BufSize, FormatString, Marker);
- Status = XenStoreWrite (Transaction, DirectoryPath, Node, Buf);
- FreePool (Buf);
+ Count = AsciiVSPrint (Buf, sizeof (Buf), FormatString, Marker);
+ ASSERT (Count > 0);
+ ASSERT (Count < sizeof (Buf));
+ if ((Count == 0) || (Count >= sizeof (Buf))) {
+ return XENSTORE_STATUS_EINVAL;
+ }
- return Status;
+ return XenStoreWrite (Transaction, DirectoryPath, Node, Buf);
}
XENSTORE_STATUS
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 04/11] OvmfPkg/XenBusDxe: Avoid Allocate in XenStoreVSPrint
2019-09-13 14:50 ` [PATCH 04/11] OvmfPkg/XenBusDxe: Avoid Allocate in XenStoreVSPrint Anthony PERARD
@ 2019-09-16 14:45 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 14:45 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> In order to be able to use XenStoreVSPrint during the
> ExitBootServices, we remove the allocation done by the function and
> use the stack instead.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/XenStore.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 5cc900190a..7b71dc156d 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1259,20 +1259,17 @@ XenStoreVSPrint (
> IN VA_LIST Marker
> )
> {
> - CHAR8 *Buf;
> - XENSTORE_STATUS Status;
> - UINTN BufSize;
> - VA_LIST Marker2;
> + CHAR8 Buf[XENSTORE_PAYLOAD_MAX];
> + UINTN Count;
>
> - VA_COPY (Marker2, Marker);
> - BufSize = SPrintLengthAsciiFormat (FormatString, Marker2) + 1;
> - VA_END (Marker2);
> - Buf = AllocateZeroPool (BufSize);
> - AsciiVSPrint (Buf, BufSize, FormatString, Marker);
> - Status = XenStoreWrite (Transaction, DirectoryPath, Node, Buf);
> - FreePool (Buf);
> + Count = AsciiVSPrint (Buf, sizeof (Buf), FormatString, Marker);
> + ASSERT (Count > 0);
> + ASSERT (Count < sizeof (Buf));
> + if ((Count == 0) || (Count >= sizeof (Buf))) {
> + return XENSTORE_STATUS_EINVAL;
> + }
>
> - return Status;
> + return XenStoreWrite (Transaction, DirectoryPath, Node, Buf);
> }
>
> XENSTORE_STATUS
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (3 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 04/11] OvmfPkg/XenBusDxe: Avoid Allocate in XenStoreVSPrint Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 15:39 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory Anthony PERARD
` (5 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
When doing an action with a path and subpath in the xenstore,
XenStoreJoin is called to generate "$path/$subpath". But this function
do an allocation of memory which isn't necessary. Instead we will
construct the path with WRITE_REQUEST and data used to generate the
path will be copied directly to the xenstore shared ring.
Also change WRITE_REQUEST.Len type, it only contain sizes and doesn't
need to be exactly 32bits.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenStore.c | 78 +++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 32 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 7b71dc156d..ca7be12d68 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -53,7 +53,7 @@
typedef struct {
CONST VOID *Data;
- UINT32 Len;
+ UINTN Len;
} WRITE_REQUEST;
/* Register callback to watch subtree (node) in the XenStore. */
@@ -260,6 +260,35 @@ XenStoreFindWatch (
return NULL;
}
+/**
+ Fill the first three slots of a WRITE_REQUEST array.
+
+ When those three slots are concatenated to generate a string, the resulting
+ string will be "$Path\0" or "$Path/$SubPath\0" if SubPath is provided.
+**/
+STATIC
+VOID
+XenStorePrepareWriteRequest (
+ IN OUT WRITE_REQUEST *WriteRequest,
+ IN CONST CHAR8 *Path,
+ IN CONST CHAR8 *SubPath OPTIONAL
+ )
+{
+ SetMem(WriteRequest, 3 * sizeof (WRITE_REQUEST), 0);
+ WriteRequest[0].Data = Path;
+ WriteRequest[0].Len = AsciiStrSize (Path);
+ if (SubPath != NULL && SubPath[0] != '\0') {
+ //
+ // Remove the \0 from the first part of the request.
+ //
+ WriteRequest[0].Len--;
+ WriteRequest[1].Data = "/";
+ WriteRequest[1].Len = 1;
+ WriteRequest[2].Data = SubPath;
+ WriteRequest[2].Len = AsciiStrSize (SubPath);
+ }
+}
+
//
// Public Utility Functions
// API comments for these methods can be found in XenStore.h
@@ -842,6 +871,7 @@ XenStoreTalkv (
@param Transaction The transaction to use for this request.
@param RequestType The type of message to send.
@param Body The body of the request.
+ @param SubPath If !NULL and not "", "/$SubPath" is append to Body.
@param LenPtr The returned length of the reply.
@param Result The returned body of the reply.
@@ -854,16 +884,16 @@ XenStoreSingle (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN enum xsd_sockmsg_type RequestType,
IN CONST CHAR8 *Body,
+ IN CONST CHAR8 *SubPath OPTIONAL,
OUT UINT32 *LenPtr OPTIONAL,
OUT VOID **Result OPTIONAL
)
{
- WRITE_REQUEST WriteRequest;
+ WRITE_REQUEST WriteRequest[3];
- WriteRequest.Data = (VOID *) Body;
- WriteRequest.Len = (UINT32)AsciiStrSize (Body);
+ XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
- return XenStoreTalkv (Transaction, RequestType, &WriteRequest, 1,
+ return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
LenPtr, Result);
}
@@ -1113,15 +1143,12 @@ XenStoreListDirectory (
OUT CONST CHAR8 ***DirectoryListPtr
)
{
- CHAR8 *Path;
CHAR8 *TempStr;
UINT32 Len = 0;
XENSTORE_STATUS Status;
- Path = XenStoreJoin (DirectoryPath, Node);
- Status = XenStoreSingle (Transaction, XS_DIRECTORY, Path, &Len,
+ Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
(VOID **) &TempStr);
- FreePool (Path);
if (Status != XENSTORE_STATUS_SUCCESS) {
return Status;
}
@@ -1160,13 +1187,11 @@ XenStoreRead (
OUT VOID **Result
)
{
- CHAR8 *Path;
VOID *Value;
XENSTORE_STATUS Status;
- Path = XenStoreJoin (DirectoryPath, Node);
- Status = XenStoreSingle (Transaction, XS_READ, Path, LenPtr, &Value);
- FreePool (Path);
+ Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
+ LenPtr, &Value);
if (Status != XENSTORE_STATUS_SUCCESS) {
return Status;
}
@@ -1183,21 +1208,13 @@ XenStoreWrite (
IN CONST CHAR8 *Str
)
{
- CHAR8 *Path;
- WRITE_REQUEST WriteRequest[2];
- XENSTORE_STATUS Status;
+ WRITE_REQUEST WriteRequest[4];
- Path = XenStoreJoin (DirectoryPath, Node);
+ XenStorePrepareWriteRequest (WriteRequest, DirectoryPath, Node);
+ WriteRequest[3].Data = Str;
+ WriteRequest[3].Len = AsciiStrLen (Str);
- WriteRequest[0].Data = (VOID *) Path;
- WriteRequest[0].Len = (UINT32)AsciiStrSize (Path);
- WriteRequest[1].Data = (VOID *) Str;
- WriteRequest[1].Len = (UINT32)AsciiStrLen (Str);
-
- Status = XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 2, NULL, NULL);
- FreePool (Path);
-
- return Status;
+ return XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 4, NULL, NULL);
}
XENSTORE_STATUS
@@ -1207,12 +1224,9 @@ XenStoreRemove (
IN CONST CHAR8 *Node
)
{
- CHAR8 *Path;
XENSTORE_STATUS Status;
- Path = XenStoreJoin (DirectoryPath, Node);
- Status = XenStoreSingle (Transaction, XS_RM, Path, NULL, NULL);
- FreePool (Path);
+ Status = XenStoreSingle (Transaction, XS_RM, DirectoryPath, Node, NULL, NULL);
return Status;
}
@@ -1226,7 +1240,7 @@ XenStoreTransactionStart (
XENSTORE_STATUS Status;
Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
- (VOID **) &IdStr);
+ NULL, (VOID **) &IdStr);
if (Status == XENSTORE_STATUS_SUCCESS) {
Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
FreePool (IdStr);
@@ -1246,7 +1260,7 @@ XenStoreTransactionEnd (
AbortStr[0] = Abort ? 'F' : 'T';
AbortStr[1] = '\0';
- return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
+ return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL, NULL);
}
XENSTORE_STATUS
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation
2019-09-13 14:50 ` [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation Anthony PERARD
@ 2019-09-16 15:39 ` Laszlo Ersek
2019-09-16 15:43 ` Laszlo Ersek
0 siblings, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 15:39 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> When doing an action with a path and subpath in the xenstore,
> XenStoreJoin is called to generate "$path/$subpath". But this function
> do an allocation of memory which isn't necessary. Instead we will
> construct the path with WRITE_REQUEST and data used to generate the
> path will be copied directly to the xenstore shared ring.
>
> Also change WRITE_REQUEST.Len type, it only contain sizes and doesn't
> need to be exactly 32bits.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/XenStore.c | 78 +++++++++++++++++++++---------------
> 1 file changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 7b71dc156d..ca7be12d68 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -53,7 +53,7 @@
>
> typedef struct {
> CONST VOID *Data;
> - UINT32 Len;
> + UINTN Len;
> } WRITE_REQUEST;
>
> /* Register callback to watch subtree (node) in the XenStore. */
> @@ -260,6 +260,35 @@ XenStoreFindWatch (
> return NULL;
> }
>
> +/**
> + Fill the first three slots of a WRITE_REQUEST array.
> +
> + When those three slots are concatenated to generate a string, the resulting
> + string will be "$Path\0" or "$Path/$SubPath\0" if SubPath is provided.
> +**/
> +STATIC
> +VOID
> +XenStorePrepareWriteRequest (
> + IN OUT WRITE_REQUEST *WriteRequest,
> + IN CONST CHAR8 *Path,
> + IN CONST CHAR8 *SubPath OPTIONAL
> + )
> +{
> + SetMem(WriteRequest, 3 * sizeof (WRITE_REQUEST), 0);
(1) ZeroMem() is more idiomatic. Also, please insert a space before the
opening paren.
> + WriteRequest[0].Data = Path;
> + WriteRequest[0].Len = AsciiStrSize (Path);
> + if (SubPath != NULL && SubPath[0] != '\0') {
> + //
> + // Remove the \0 from the first part of the request.
> + //
> + WriteRequest[0].Len--;
> + WriteRequest[1].Data = "/";
> + WriteRequest[1].Len = 1;
> + WriteRequest[2].Data = SubPath;
> + WriteRequest[2].Len = AsciiStrSize (SubPath);
> + }
> +}
> +
So this suggests that only the last element in the array should point to
a NUL-terminated string. Strings pointed-to by earlier elements in the
array should not be NUL-terminated. Is that correct?
> //
> // Public Utility Functions
> // API comments for these methods can be found in XenStore.h
> @@ -842,6 +871,7 @@ XenStoreTalkv (
> @param Transaction The transaction to use for this request.
> @param RequestType The type of message to send.
> @param Body The body of the request.
> + @param SubPath If !NULL and not "", "/$SubPath" is append to Body.
> @param LenPtr The returned length of the reply.
> @param Result The returned body of the reply.
>
> @@ -854,16 +884,16 @@ XenStoreSingle (
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN enum xsd_sockmsg_type RequestType,
> IN CONST CHAR8 *Body,
> + IN CONST CHAR8 *SubPath OPTIONAL,
> OUT UINT32 *LenPtr OPTIONAL,
> OUT VOID **Result OPTIONAL
> )
> {
> - WRITE_REQUEST WriteRequest;
> + WRITE_REQUEST WriteRequest[3];
>
> - WriteRequest.Data = (VOID *) Body;
> - WriteRequest.Len = (UINT32)AsciiStrSize (Body);
> + XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
>
> - return XenStoreTalkv (Transaction, RequestType, &WriteRequest, 1,
> + return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
> LenPtr, Result);
(2) It would be slightly more idiomatic to pass
ARRAY_SIZE (WriteRequest)
in place of the naked 3.
> }
>
> @@ -1113,15 +1143,12 @@ XenStoreListDirectory (
> OUT CONST CHAR8 ***DirectoryListPtr
> )
> {
> - CHAR8 *Path;
> CHAR8 *TempStr;
> UINT32 Len = 0;
> XENSTORE_STATUS Status;
>
> - Path = XenStoreJoin (DirectoryPath, Node);
> - Status = XenStoreSingle (Transaction, XS_DIRECTORY, Path, &Len,
> + Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
> (VOID **) &TempStr);
> - FreePool (Path);
> if (Status != XENSTORE_STATUS_SUCCESS) {
> return Status;
> }
> @@ -1160,13 +1187,11 @@ XenStoreRead (
> OUT VOID **Result
> )
> {
> - CHAR8 *Path;
> VOID *Value;
> XENSTORE_STATUS Status;
>
> - Path = XenStoreJoin (DirectoryPath, Node);
> - Status = XenStoreSingle (Transaction, XS_READ, Path, LenPtr, &Value);
> - FreePool (Path);
> + Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
> + LenPtr, &Value);
(3) Indentation.
> if (Status != XENSTORE_STATUS_SUCCESS) {
> return Status;
> }
> @@ -1183,21 +1208,13 @@ XenStoreWrite (
> IN CONST CHAR8 *Str
> )
> {
> - CHAR8 *Path;
> - WRITE_REQUEST WriteRequest[2];
> - XENSTORE_STATUS Status;
> + WRITE_REQUEST WriteRequest[4];
>
> - Path = XenStoreJoin (DirectoryPath, Node);
> + XenStorePrepareWriteRequest (WriteRequest, DirectoryPath, Node);
> + WriteRequest[3].Data = Str;
> + WriteRequest[3].Len = AsciiStrLen (Str);
Now we have two strings, pointed-to by elements in the array, that are
NUL-terminated: the element at offset 2, and the one at offset 3. Is
that intentional? Is that part of the message framing?
Hmmm... From the original code:
>
> - WriteRequest[0].Data = (VOID *) Path;
> - WriteRequest[0].Len = (UINT32)AsciiStrSize (Path);
> - WriteRequest[1].Data = (VOID *) Str;
> - WriteRequest[1].Len = (UINT32)AsciiStrLen (Str);
That seems to be the case. I guess the first run (offsets 0 through 2)
is parsed until the first NUL is encountered, for "path", then the
second run (3 and onwards) is parsed until the second NUL for "data".
Sounds plausible; OK.
> -
> - Status = XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 2, NULL, NULL);
> - FreePool (Path);
> -
> - return Status;
> + return XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 4, NULL, NULL);
(4) Please use ARRAY_SIZE(); it's more robust.
> }
>
> XENSTORE_STATUS
> @@ -1207,12 +1224,9 @@ XenStoreRemove (
> IN CONST CHAR8 *Node
> )
> {
> - CHAR8 *Path;
> XENSTORE_STATUS Status;
>
> - Path = XenStoreJoin (DirectoryPath, Node);
> - Status = XenStoreSingle (Transaction, XS_RM, Path, NULL, NULL);
> - FreePool (Path);
> + Status = XenStoreSingle (Transaction, XS_RM, DirectoryPath, Node, NULL, NULL);
>
> return Status;
> }
> @@ -1226,7 +1240,7 @@ XenStoreTransactionStart (
> XENSTORE_STATUS Status;
>
> Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
> - (VOID **) &IdStr);
> + NULL, (VOID **) &IdStr);
(5) Indentation.
> if (Status == XENSTORE_STATUS_SUCCESS) {
> Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
> FreePool (IdStr);
> @@ -1246,7 +1260,7 @@ XenStoreTransactionEnd (
> AbortStr[0] = Abort ? 'F' : 'T';
> AbortStr[1] = '\0';
>
> - return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
> + return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL, NULL);
> }
>
> XENSTORE_STATUS
>
With the above addressed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation
2019-09-16 15:39 ` [edk2-devel] " Laszlo Ersek
@ 2019-09-16 15:43 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 15:43 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/16/19 17:39, Laszlo Ersek wrote:
> On 09/13/19 16:50, Anthony PERARD wrote:
>> When doing an action with a path and subpath in the xenstore,
>> XenStoreJoin is called to generate "$path/$subpath". But this function
>> do an allocation of memory which isn't necessary. Instead we will
>> construct the path with WRITE_REQUEST and data used to generate the
>> path will be copied directly to the xenstore shared ring.
>>
>> Also change WRITE_REQUEST.Len type, it only contain sizes and doesn't
>> need to be exactly 32bits.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>> OvmfPkg/XenBusDxe/XenStore.c | 78 +++++++++++++++++++++---------------
>> 1 file changed, 46 insertions(+), 32 deletions(-)
>>
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
>> index 7b71dc156d..ca7be12d68 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>> @@ -53,7 +53,7 @@
>>
>> typedef struct {
>> CONST VOID *Data;
>> - UINT32 Len;
>> + UINTN Len;
>> } WRITE_REQUEST;
>>
>> /* Register callback to watch subtree (node) in the XenStore. */
>> @@ -260,6 +260,35 @@ XenStoreFindWatch (
>> return NULL;
>> }
>>
>> +/**
>> + Fill the first three slots of a WRITE_REQUEST array.
>> +
>> + When those three slots are concatenated to generate a string, the resulting
>> + string will be "$Path\0" or "$Path/$SubPath\0" if SubPath is provided.
>> +**/
>> +STATIC
>> +VOID
>> +XenStorePrepareWriteRequest (
>> + IN OUT WRITE_REQUEST *WriteRequest,
(6) I think this could be just OUT -- we start by zeroing it out.
>> + IN CONST CHAR8 *Path,
>> + IN CONST CHAR8 *SubPath OPTIONAL
>> + )
>> +{
>> + SetMem(WriteRequest, 3 * sizeof (WRITE_REQUEST), 0);
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (4 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 15:41 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions Anthony PERARD
` (4 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
This patch rework XenStoreProcessMessage in order to avoid memory
allocation when a reply is expected. Instead of allocating a buffer
for this reply, we are going to copy to a buffer passed by the caller.
For messages that aren't fully received, they will be stored in a
buffer that have been allocated at the initialisation of the driver.
A temporary memory allocation is made in XenStoreTalkv but that will
be removed in a further patch.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenStore.c | 297 +++++++++++++++--------------------
1 file changed, 130 insertions(+), 167 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index ca7be12d68..004d3b6022 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -72,27 +72,6 @@ struct _XENSTORE_WATCH
#define XENSTORE_WATCH_FROM_LINK(l) \
CR (l, XENSTORE_WATCH, Link, XENSTORE_WATCH_SIGNATURE)
-
-/**
- * Structure capturing messages received from the XenStore service.
- */
-#define XENSTORE_MESSAGE_SIGNATURE SIGNATURE_32 ('X', 'S', 's', 'm')
-typedef struct {
- UINT32 Signature;
- LIST_ENTRY Link;
-
- struct xsd_sockmsg Header;
-
- union {
- /* Queued replies. */
- struct {
- CHAR8 *Body;
- } Reply;
- } u;
-} XENSTORE_MESSAGE;
-#define XENSTORE_MESSAGE_FROM_LINK(r) \
- CR (r, XENSTORE_MESSAGE, Link, XENSTORE_MESSAGE_SIGNATURE)
-
/**
* Container for all XenStore related state.
*/
@@ -105,21 +84,6 @@ typedef struct {
XENBUS_DEVICE *Dev;
- /**
- * A list of replies to our requests.
- *
- * The reply list is filled by xs_rcv_thread(). It
- * is consumed by the context that issued the request
- * to which a reply is made. The requester blocks in
- * XenStoreReadReply ().
- *
- * /note Only one requesting context can be active at a time.
- */
- LIST_ENTRY ReplyList;
-
- /** Lock protecting the reply list. */
- EFI_LOCK ReplyLock;
-
/**
* List of registered watches.
*/
@@ -136,6 +100,13 @@ typedef struct {
/** Handle for XenStore events. */
EFI_EVENT EventChannelEvent;
+
+ /** Buffer used to copy payloads from the xenstore ring */
+ // The + 1 is to allow to have a \0.
+ CHAR8 Buffer[XENSTORE_PAYLOAD_MAX + 1];
+
+ /** ID used when sending messages to xenstored */
+ UINTN NextRequestId;
} XENSTORE_PRIVATE;
//
@@ -148,6 +119,12 @@ static XENSTORE_PRIVATE xs;
// Private Utility Functions
//
+STATIC
+XENSTORE_STATUS
+XenStoreGetError (
+ CONST CHAR8 *ErrorStr
+ );
+
/**
Count and optionally record pointers to a number of NUL terminated
strings in a buffer.
@@ -613,70 +590,106 @@ XenStoreReadStore (
Block reading the next message from the XenStore service and
process the result.
+ @param ExpectedRequestId Block until a reply to with this ID is seen.
+ @param ExpectedTransactionId Idem, but should also match this ID.
+ @param BufferSize IN: size of the buffer
+ OUT: The returned length of the reply.
+ @param Buffer The returned body of the reply.
+
@return XENSTORE_STATUS_SUCCESS on success. Otherwise an errno value
indicating the type of failure encountered.
**/
STATIC
XENSTORE_STATUS
XenStoreProcessMessage (
- VOID
+ IN UINT32 ExpectedRequestId,
+ IN UINT32 ExpectedTransactionId,
+ IN OUT UINTN *BufferSize OPTIONAL,
+ IN OUT CHAR8 *Buffer OPTIONAL
)
{
- XENSTORE_MESSAGE *Message;
- CHAR8 *Body;
- XENSTORE_STATUS Status;
-
- Message = AllocateZeroPool (sizeof (XENSTORE_MESSAGE));
- Message->Signature = XENSTORE_MESSAGE_SIGNATURE;
- Status = XenStoreReadStore (&Message->Header, sizeof (Message->Header));
- if (Status != XENSTORE_STATUS_SUCCESS) {
- FreePool (Message);
- DEBUG ((EFI_D_ERROR, "XenStore: Error read store (%d)\n", Status));
- return Status;
- }
-
- Body = AllocatePool (Message->Header.len + 1);
- Status = XenStoreReadStore (Body, Message->Header.len);
- if (Status != XENSTORE_STATUS_SUCCESS) {
- FreePool (Body);
- FreePool (Message);
- DEBUG ((EFI_D_ERROR, "XenStore: Error read store (%d)\n", Status));
- return Status;
- }
- Body[Message->Header.len] = '\0';
+ struct xsd_sockmsg Header;
+ CHAR8 *Payload;
+ XENSTORE_STATUS Status;
- if (Message->Header.type == XS_WATCH_EVENT) {
- CONST CHAR8 *WatchEventPath;
- CONST CHAR8 *WatchEventToken;
- VOID *ConvertedToken;
- XENSTORE_WATCH *Watch;
+ while (TRUE) {
- //
- // Parse WATCH_EVENT messages
- // <path>\0<token>\0
- //
- WatchEventPath = Body;
- WatchEventToken = WatchEventPath + AsciiStrSize (WatchEventPath);
+ Status = XenStoreReadStore (&Header, sizeof (Header));
+ if (Status != XENSTORE_STATUS_SUCCESS) {
+ DEBUG ((DEBUG_ERROR, "XenStore: Error read store (%d)\n", Status));
+ return Status;
+ }
- ConvertedToken = (VOID *) AsciiStrHexToUintn (WatchEventToken);
+ ASSERT (Header.len <= XENSTORE_PAYLOAD_MAX);
+ if (Header.len > XENSTORE_PAYLOAD_MAX) {
+ DEBUG ((DEBUG_ERROR, "XenStore: Message payload over %d (is %d)\n",
+ XENSTORE_PAYLOAD_MAX, Header.len));
+ Header.len = XENSTORE_PAYLOAD_MAX;
+ }
- EfiAcquireLock (&xs.RegisteredWatchesLock);
- Watch = XenStoreFindWatch (ConvertedToken);
- DEBUG ((DEBUG_INFO, "XenStore: Watch event %a\n", WatchEventToken));
- if (Watch != NULL) {
- Watch->Triggered = TRUE;
- } else {
- DEBUG ((EFI_D_WARN, "XenStore: Watch handle %a not found\n",
- WatchEventToken));
+ Payload = xs.Buffer;
+ Status = XenStoreReadStore (Payload, Header.len);
+ if (Status != XENSTORE_STATUS_SUCCESS) {
+ DEBUG ((DEBUG_ERROR, "XenStore: Error read store (%d)\n", Status));
+ return Status;
}
- EfiReleaseLock (&xs.RegisteredWatchesLock);
- FreePool (Message);
- FreePool (Body);
- } else {
- Message->u.Reply.Body = Body;
- EfiAcquireLock (&xs.ReplyLock);
- InsertTailList (&xs.ReplyList, &Message->Link);
- EfiReleaseLock (&xs.ReplyLock);
+ Payload[Header.len] = '\0';
+
+ if (Header.type == XS_WATCH_EVENT) {
+ CONST CHAR8 *WatchEventPath;
+ CONST CHAR8 *WatchEventToken;
+ VOID *ConvertedToken;
+ XENSTORE_WATCH *Watch;
+
+ //
+ // Parse WATCH_EVENT messages
+ // <path>\0<token>\0
+ //
+ WatchEventPath = Payload;
+ WatchEventToken = WatchEventPath + AsciiStrSize (WatchEventPath);
+
+ ConvertedToken = (VOID *) AsciiStrHexToUintn (WatchEventToken);
+
+ EfiAcquireLock (&xs.RegisteredWatchesLock);
+ Watch = XenStoreFindWatch (ConvertedToken);
+ DEBUG ((DEBUG_INFO, "XenStore: Watch event %a\n", WatchEventToken));
+ if (Watch != NULL) {
+ Watch->Triggered = TRUE;
+ } else {
+ DEBUG ((DEBUG_WARN, "XenStore: Watch handle %a not found\n",
+ WatchEventToken));
+ }
+ EfiReleaseLock (&xs.RegisteredWatchesLock);
+
+ if (Header.req_id == ExpectedRequestId
+ && Header.tx_id == ExpectedTransactionId
+ && Buffer == NULL) {
+ //
+ // We were waiting for a watch event
+ //
+ return XENSTORE_STATUS_SUCCESS;
+ }
+ } else if (Header.req_id == ExpectedRequestId
+ && Header.tx_id == ExpectedTransactionId) {
+ Status = XENSTORE_STATUS_SUCCESS;
+ if (Header.type == XS_ERROR) {
+ Status = XenStoreGetError (Payload);
+ } else if (Buffer != NULL) {
+ ASSERT (BufferSize != NULL);
+ ASSERT (*BufferSize >= Header.len);
+ CopyMem (Buffer, Payload, MIN (Header.len + 1, *BufferSize));
+ *BufferSize = Header.len;
+ } else {
+ //
+ // Payload should be "OK" if the function sending a request doesn't
+ // expect a reply.
+ //
+ ASSERT (Header.len == 3);
+ ASSERT (AsciiStrCmp (Payload, "OK") == 0);
+ }
+ return Status;
+ }
+
}
return XENSTORE_STATUS_SUCCESS;
@@ -736,51 +749,6 @@ XenStoreGetError (
return XENSTORE_STATUS_EINVAL;
}
-/**
- Block waiting for a reply to a message request.
-
- @param TypePtr The returned type of the reply.
- @param LenPtr The returned body length of the reply.
- @param Result The returned body of the reply.
-**/
-STATIC
-XENSTORE_STATUS
-XenStoreReadReply (
- OUT enum xsd_sockmsg_type *TypePtr,
- OUT UINT32 *LenPtr OPTIONAL,
- OUT VOID **Result
- )
-{
- XENSTORE_MESSAGE *Message;
- LIST_ENTRY *Entry;
- CHAR8 *Body;
-
- while (IsListEmpty (&xs.ReplyList)) {
- XENSTORE_STATUS Status;
- Status = XenStoreProcessMessage ();
- if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
- DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
- Status));
- return Status;
- }
- }
- EfiAcquireLock (&xs.ReplyLock);
- Entry = GetFirstNode (&xs.ReplyList);
- Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
- RemoveEntryList (Entry);
- EfiReleaseLock (&xs.ReplyLock);
-
- *TypePtr = Message->Header.type;
- if (LenPtr != NULL) {
- *LenPtr = Message->Header.len;
- }
- Body = Message->u.Reply.Body;
-
- FreePool (Message);
- *Result = Body;
- return XENSTORE_STATUS_SUCCESS;
-}
-
/**
Send a message with an optionally muti-part body to the XenStore service.
@@ -806,16 +774,17 @@ XenStoreTalkv (
)
{
struct xsd_sockmsg Message;
- void *Return = NULL;
- UINT32 Index;
- XENSTORE_STATUS Status;
+ UINTN Index;
+ XENSTORE_STATUS Status;
+ VOID *Buffer;
+ UINTN BufferSize;
if (Transaction == XST_NIL) {
Message.tx_id = 0;
} else {
Message.tx_id = Transaction->Id;
}
- Message.req_id = 0;
+ Message.req_id = xs.NextRequestId++;
Message.type = RequestType;
Message.len = 0;
for (Index = 0; Index < NumRequests; Index++) {
@@ -836,29 +805,36 @@ XenStoreTalkv (
}
}
- Status = XenStoreReadReply ((enum xsd_sockmsg_type *)&Message.type, LenPtr, &Return);
-
-Error:
- if (Status != XENSTORE_STATUS_SUCCESS) {
- return Status;
+ if (ResultPtr) {
+ Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
+ BufferSize = XENSTORE_PAYLOAD_MAX;
+ } else {
+ Buffer = NULL;
+ BufferSize = 0;
}
- if (Message.type == XS_ERROR) {
- Status = XenStoreGetError (Return);
- FreePool (Return);
+ //
+ // Wait for a reply to our request
+ //
+ Status = XenStoreProcessMessage (Message.req_id, Message.tx_id,
+ &BufferSize, Buffer);
+
+ if (Status != XENSTORE_STATUS_SUCCESS) {
+ DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
+ Status));
+ FreePool (Buffer);
return Status;
}
- /* Reply is either error or an echo of our request message type. */
- ASSERT ((enum xsd_sockmsg_type)Message.type == RequestType);
-
if (ResultPtr) {
- *ResultPtr = Return;
- } else {
- FreePool (Return);
+ *ResultPtr = Buffer;
+ if (LenPtr) {
+ *LenPtr = BufferSize;
+ }
}
- return XENSTORE_STATUS_SUCCESS;
+Error:
+ return Status;
}
/**
@@ -975,7 +951,7 @@ XenStoreWaitWatch (
return XENSTORE_STATUS_SUCCESS;
}
- Status = XenStoreProcessMessage ();
+ Status = XenStoreProcessMessage (0, 0, NULL, NULL);
if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
return Status;
}
@@ -1060,12 +1036,12 @@ XenStoreInit (
DEBUG ((EFI_D_INFO, "XenBusInit: XenBus rings @%p, event channel %x\n",
xs.XenStore, xs.EventChannel));
- InitializeListHead (&xs.ReplyList);
InitializeListHead (&xs.RegisteredWatches);
- EfiInitializeLock (&xs.ReplyLock, TPL_NOTIFY);
EfiInitializeLock (&xs.RegisteredWatchesLock, TPL_NOTIFY);
+ xs.NextRequestId = 1;
+
/* Initialize the shared memory rings to talk to xenstored */
Status = XenStoreInitComms (&xs);
@@ -1095,19 +1071,6 @@ XenStoreDeinit (
}
}
- if (!IsListEmpty (&xs.ReplyList)) {
- XENSTORE_MESSAGE *Message;
- LIST_ENTRY *Entry;
- Entry = GetFirstNode (&xs.ReplyList);
- while (!IsNull (&xs.ReplyList, Entry)) {
- Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
- Entry = GetNextNode (&xs.ReplyList, Entry);
- RemoveEntryList (&Message->Link);
- FreePool (Message->u.Reply.Body);
- FreePool (Message);
- }
- }
-
gBS->CloseEvent (xs.EventChannelEvent);
if (xs.XenStore->server_features & XENSTORE_SERVER_FEATURE_RECONNECTION) {
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory
2019-09-13 14:50 ` [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory Anthony PERARD
@ 2019-09-16 15:41 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 15:41 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> This patch rework XenStoreProcessMessage in order to avoid memory
> allocation when a reply is expected. Instead of allocating a buffer
> for this reply, we are going to copy to a buffer passed by the caller.
> For messages that aren't fully received, they will be stored in a
> buffer that have been allocated at the initialisation of the driver.
>
> A temporary memory allocation is made in XenStoreTalkv but that will
> be removed in a further patch.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/XenStore.c | 297 +++++++++++++++--------------------
> 1 file changed, 130 insertions(+), 167 deletions(-)
Sorry, too big for a detailed review, and I'd like to go through the
series today. So, based on the diffstat,
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index ca7be12d68..004d3b6022 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -72,27 +72,6 @@ struct _XENSTORE_WATCH
> #define XENSTORE_WATCH_FROM_LINK(l) \
> CR (l, XENSTORE_WATCH, Link, XENSTORE_WATCH_SIGNATURE)
>
> -
> -/**
> - * Structure capturing messages received from the XenStore service.
> - */
> -#define XENSTORE_MESSAGE_SIGNATURE SIGNATURE_32 ('X', 'S', 's', 'm')
> -typedef struct {
> - UINT32 Signature;
> - LIST_ENTRY Link;
> -
> - struct xsd_sockmsg Header;
> -
> - union {
> - /* Queued replies. */
> - struct {
> - CHAR8 *Body;
> - } Reply;
> - } u;
> -} XENSTORE_MESSAGE;
> -#define XENSTORE_MESSAGE_FROM_LINK(r) \
> - CR (r, XENSTORE_MESSAGE, Link, XENSTORE_MESSAGE_SIGNATURE)
> -
> /**
> * Container for all XenStore related state.
> */
> @@ -105,21 +84,6 @@ typedef struct {
>
> XENBUS_DEVICE *Dev;
>
> - /**
> - * A list of replies to our requests.
> - *
> - * The reply list is filled by xs_rcv_thread(). It
> - * is consumed by the context that issued the request
> - * to which a reply is made. The requester blocks in
> - * XenStoreReadReply ().
> - *
> - * /note Only one requesting context can be active at a time.
> - */
> - LIST_ENTRY ReplyList;
> -
> - /** Lock protecting the reply list. */
> - EFI_LOCK ReplyLock;
> -
> /**
> * List of registered watches.
> */
> @@ -136,6 +100,13 @@ typedef struct {
>
> /** Handle for XenStore events. */
> EFI_EVENT EventChannelEvent;
> +
> + /** Buffer used to copy payloads from the xenstore ring */
> + // The + 1 is to allow to have a \0.
> + CHAR8 Buffer[XENSTORE_PAYLOAD_MAX + 1];
> +
> + /** ID used when sending messages to xenstored */
> + UINTN NextRequestId;
> } XENSTORE_PRIVATE;
>
> //
> @@ -148,6 +119,12 @@ static XENSTORE_PRIVATE xs;
> // Private Utility Functions
> //
>
> +STATIC
> +XENSTORE_STATUS
> +XenStoreGetError (
> + CONST CHAR8 *ErrorStr
> + );
> +
> /**
> Count and optionally record pointers to a number of NUL terminated
> strings in a buffer.
> @@ -613,70 +590,106 @@ XenStoreReadStore (
> Block reading the next message from the XenStore service and
> process the result.
>
> + @param ExpectedRequestId Block until a reply to with this ID is seen.
> + @param ExpectedTransactionId Idem, but should also match this ID.
> + @param BufferSize IN: size of the buffer
> + OUT: The returned length of the reply.
> + @param Buffer The returned body of the reply.
> +
> @return XENSTORE_STATUS_SUCCESS on success. Otherwise an errno value
> indicating the type of failure encountered.
> **/
> STATIC
> XENSTORE_STATUS
> XenStoreProcessMessage (
> - VOID
> + IN UINT32 ExpectedRequestId,
> + IN UINT32 ExpectedTransactionId,
> + IN OUT UINTN *BufferSize OPTIONAL,
> + IN OUT CHAR8 *Buffer OPTIONAL
> )
> {
> - XENSTORE_MESSAGE *Message;
> - CHAR8 *Body;
> - XENSTORE_STATUS Status;
> -
> - Message = AllocateZeroPool (sizeof (XENSTORE_MESSAGE));
> - Message->Signature = XENSTORE_MESSAGE_SIGNATURE;
> - Status = XenStoreReadStore (&Message->Header, sizeof (Message->Header));
> - if (Status != XENSTORE_STATUS_SUCCESS) {
> - FreePool (Message);
> - DEBUG ((EFI_D_ERROR, "XenStore: Error read store (%d)\n", Status));
> - return Status;
> - }
> -
> - Body = AllocatePool (Message->Header.len + 1);
> - Status = XenStoreReadStore (Body, Message->Header.len);
> - if (Status != XENSTORE_STATUS_SUCCESS) {
> - FreePool (Body);
> - FreePool (Message);
> - DEBUG ((EFI_D_ERROR, "XenStore: Error read store (%d)\n", Status));
> - return Status;
> - }
> - Body[Message->Header.len] = '\0';
> + struct xsd_sockmsg Header;
> + CHAR8 *Payload;
> + XENSTORE_STATUS Status;
>
> - if (Message->Header.type == XS_WATCH_EVENT) {
> - CONST CHAR8 *WatchEventPath;
> - CONST CHAR8 *WatchEventToken;
> - VOID *ConvertedToken;
> - XENSTORE_WATCH *Watch;
> + while (TRUE) {
>
> - //
> - // Parse WATCH_EVENT messages
> - // <path>\0<token>\0
> - //
> - WatchEventPath = Body;
> - WatchEventToken = WatchEventPath + AsciiStrSize (WatchEventPath);
> + Status = XenStoreReadStore (&Header, sizeof (Header));
> + if (Status != XENSTORE_STATUS_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "XenStore: Error read store (%d)\n", Status));
> + return Status;
> + }
>
> - ConvertedToken = (VOID *) AsciiStrHexToUintn (WatchEventToken);
> + ASSERT (Header.len <= XENSTORE_PAYLOAD_MAX);
> + if (Header.len > XENSTORE_PAYLOAD_MAX) {
> + DEBUG ((DEBUG_ERROR, "XenStore: Message payload over %d (is %d)\n",
> + XENSTORE_PAYLOAD_MAX, Header.len));
> + Header.len = XENSTORE_PAYLOAD_MAX;
> + }
>
> - EfiAcquireLock (&xs.RegisteredWatchesLock);
> - Watch = XenStoreFindWatch (ConvertedToken);
> - DEBUG ((DEBUG_INFO, "XenStore: Watch event %a\n", WatchEventToken));
> - if (Watch != NULL) {
> - Watch->Triggered = TRUE;
> - } else {
> - DEBUG ((EFI_D_WARN, "XenStore: Watch handle %a not found\n",
> - WatchEventToken));
> + Payload = xs.Buffer;
> + Status = XenStoreReadStore (Payload, Header.len);
> + if (Status != XENSTORE_STATUS_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "XenStore: Error read store (%d)\n", Status));
> + return Status;
> }
> - EfiReleaseLock (&xs.RegisteredWatchesLock);
> - FreePool (Message);
> - FreePool (Body);
> - } else {
> - Message->u.Reply.Body = Body;
> - EfiAcquireLock (&xs.ReplyLock);
> - InsertTailList (&xs.ReplyList, &Message->Link);
> - EfiReleaseLock (&xs.ReplyLock);
> + Payload[Header.len] = '\0';
> +
> + if (Header.type == XS_WATCH_EVENT) {
> + CONST CHAR8 *WatchEventPath;
> + CONST CHAR8 *WatchEventToken;
> + VOID *ConvertedToken;
> + XENSTORE_WATCH *Watch;
> +
> + //
> + // Parse WATCH_EVENT messages
> + // <path>\0<token>\0
> + //
> + WatchEventPath = Payload;
> + WatchEventToken = WatchEventPath + AsciiStrSize (WatchEventPath);
> +
> + ConvertedToken = (VOID *) AsciiStrHexToUintn (WatchEventToken);
> +
> + EfiAcquireLock (&xs.RegisteredWatchesLock);
> + Watch = XenStoreFindWatch (ConvertedToken);
> + DEBUG ((DEBUG_INFO, "XenStore: Watch event %a\n", WatchEventToken));
> + if (Watch != NULL) {
> + Watch->Triggered = TRUE;
> + } else {
> + DEBUG ((DEBUG_WARN, "XenStore: Watch handle %a not found\n",
> + WatchEventToken));
> + }
> + EfiReleaseLock (&xs.RegisteredWatchesLock);
> +
> + if (Header.req_id == ExpectedRequestId
> + && Header.tx_id == ExpectedTransactionId
> + && Buffer == NULL) {
> + //
> + // We were waiting for a watch event
> + //
> + return XENSTORE_STATUS_SUCCESS;
> + }
> + } else if (Header.req_id == ExpectedRequestId
> + && Header.tx_id == ExpectedTransactionId) {
> + Status = XENSTORE_STATUS_SUCCESS;
> + if (Header.type == XS_ERROR) {
> + Status = XenStoreGetError (Payload);
> + } else if (Buffer != NULL) {
> + ASSERT (BufferSize != NULL);
> + ASSERT (*BufferSize >= Header.len);
> + CopyMem (Buffer, Payload, MIN (Header.len + 1, *BufferSize));
> + *BufferSize = Header.len;
> + } else {
> + //
> + // Payload should be "OK" if the function sending a request doesn't
> + // expect a reply.
> + //
> + ASSERT (Header.len == 3);
> + ASSERT (AsciiStrCmp (Payload, "OK") == 0);
> + }
> + return Status;
> + }
> +
> }
>
> return XENSTORE_STATUS_SUCCESS;
> @@ -736,51 +749,6 @@ XenStoreGetError (
> return XENSTORE_STATUS_EINVAL;
> }
>
> -/**
> - Block waiting for a reply to a message request.
> -
> - @param TypePtr The returned type of the reply.
> - @param LenPtr The returned body length of the reply.
> - @param Result The returned body of the reply.
> -**/
> -STATIC
> -XENSTORE_STATUS
> -XenStoreReadReply (
> - OUT enum xsd_sockmsg_type *TypePtr,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID **Result
> - )
> -{
> - XENSTORE_MESSAGE *Message;
> - LIST_ENTRY *Entry;
> - CHAR8 *Body;
> -
> - while (IsListEmpty (&xs.ReplyList)) {
> - XENSTORE_STATUS Status;
> - Status = XenStoreProcessMessage ();
> - if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
> - DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
> - Status));
> - return Status;
> - }
> - }
> - EfiAcquireLock (&xs.ReplyLock);
> - Entry = GetFirstNode (&xs.ReplyList);
> - Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
> - RemoveEntryList (Entry);
> - EfiReleaseLock (&xs.ReplyLock);
> -
> - *TypePtr = Message->Header.type;
> - if (LenPtr != NULL) {
> - *LenPtr = Message->Header.len;
> - }
> - Body = Message->u.Reply.Body;
> -
> - FreePool (Message);
> - *Result = Body;
> - return XENSTORE_STATUS_SUCCESS;
> -}
> -
> /**
> Send a message with an optionally muti-part body to the XenStore service.
>
> @@ -806,16 +774,17 @@ XenStoreTalkv (
> )
> {
> struct xsd_sockmsg Message;
> - void *Return = NULL;
> - UINT32 Index;
> - XENSTORE_STATUS Status;
> + UINTN Index;
> + XENSTORE_STATUS Status;
> + VOID *Buffer;
> + UINTN BufferSize;
>
> if (Transaction == XST_NIL) {
> Message.tx_id = 0;
> } else {
> Message.tx_id = Transaction->Id;
> }
> - Message.req_id = 0;
> + Message.req_id = xs.NextRequestId++;
> Message.type = RequestType;
> Message.len = 0;
> for (Index = 0; Index < NumRequests; Index++) {
> @@ -836,29 +805,36 @@ XenStoreTalkv (
> }
> }
>
> - Status = XenStoreReadReply ((enum xsd_sockmsg_type *)&Message.type, LenPtr, &Return);
> -
> -Error:
> - if (Status != XENSTORE_STATUS_SUCCESS) {
> - return Status;
> + if (ResultPtr) {
> + Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
> + BufferSize = XENSTORE_PAYLOAD_MAX;
> + } else {
> + Buffer = NULL;
> + BufferSize = 0;
> }
>
> - if (Message.type == XS_ERROR) {
> - Status = XenStoreGetError (Return);
> - FreePool (Return);
> + //
> + // Wait for a reply to our request
> + //
> + Status = XenStoreProcessMessage (Message.req_id, Message.tx_id,
> + &BufferSize, Buffer);
> +
> + if (Status != XENSTORE_STATUS_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
> + Status));
> + FreePool (Buffer);
> return Status;
> }
>
> - /* Reply is either error or an echo of our request message type. */
> - ASSERT ((enum xsd_sockmsg_type)Message.type == RequestType);
> -
> if (ResultPtr) {
> - *ResultPtr = Return;
> - } else {
> - FreePool (Return);
> + *ResultPtr = Buffer;
> + if (LenPtr) {
> + *LenPtr = BufferSize;
> + }
> }
>
> - return XENSTORE_STATUS_SUCCESS;
> +Error:
> + return Status;
> }
>
> /**
> @@ -975,7 +951,7 @@ XenStoreWaitWatch (
> return XENSTORE_STATUS_SUCCESS;
> }
>
> - Status = XenStoreProcessMessage ();
> + Status = XenStoreProcessMessage (0, 0, NULL, NULL);
> if (Status != XENSTORE_STATUS_SUCCESS && Status != XENSTORE_STATUS_EAGAIN) {
> return Status;
> }
> @@ -1060,12 +1036,12 @@ XenStoreInit (
> DEBUG ((EFI_D_INFO, "XenBusInit: XenBus rings @%p, event channel %x\n",
> xs.XenStore, xs.EventChannel));
>
> - InitializeListHead (&xs.ReplyList);
> InitializeListHead (&xs.RegisteredWatches);
>
> - EfiInitializeLock (&xs.ReplyLock, TPL_NOTIFY);
> EfiInitializeLock (&xs.RegisteredWatchesLock, TPL_NOTIFY);
>
> + xs.NextRequestId = 1;
> +
> /* Initialize the shared memory rings to talk to xenstored */
> Status = XenStoreInitComms (&xs);
>
> @@ -1095,19 +1071,6 @@ XenStoreDeinit (
> }
> }
>
> - if (!IsListEmpty (&xs.ReplyList)) {
> - XENSTORE_MESSAGE *Message;
> - LIST_ENTRY *Entry;
> - Entry = GetFirstNode (&xs.ReplyList);
> - while (!IsNull (&xs.ReplyList, Entry)) {
> - Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
> - Entry = GetNextNode (&xs.ReplyList, Entry);
> - RemoveEntryList (&Message->Link);
> - FreePool (Message->u.Reply.Body);
> - FreePool (Message);
> - }
> - }
> -
> gBS->CloseEvent (xs.EventChannelEvent);
>
> if (xs.XenStore->server_features & XENSTORE_SERVER_FEATURE_RECONNECTION) {
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (5 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 16:11 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory Anthony PERARD
` (3 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
We will use a buffer on the stack instead of allocating memory for
internal functions that are expecting a reply from xenstore.
The external interface XENBUS_PROTOCOL isn't changed yet, so
allocation are made for XsRead and XsBackendRead.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenBus.c | 40 ++++++------
OvmfPkg/XenBusDxe/XenStore.c | 115 ++++++++++++++++++++---------------
OvmfPkg/XenBusDxe/XenStore.h | 17 +++---
3 files changed, 95 insertions(+), 77 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
index bb8ddbc4d4..78835ec7b3 100644
--- a/OvmfPkg/XenBusDxe/XenBus.c
+++ b/OvmfPkg/XenBusDxe/XenBus.c
@@ -89,19 +89,18 @@ XenBusReadDriverState (
IN CONST CHAR8 *Path
)
{
- XenbusState State;
- CHAR8 *Ptr = NULL;
+ XenbusState State;
+ CHAR8 Buffer[4];
+ UINTN BufferSize;
XENSTORE_STATUS Status;
- Status = XenStoreRead (XST_NIL, Path, "state", NULL, (VOID **)&Ptr);
+ BufferSize = sizeof (Buffer) - 1;
+ Status = XenStoreRead (XST_NIL, Path, "state", &BufferSize, Buffer);
if (Status != XENSTORE_STATUS_SUCCESS) {
State = XenbusStateClosed;
} else {
- State = AsciiStrDecimalToUintn (Ptr);
- }
-
- if (Ptr != NULL) {
- FreePool (Ptr);
+ Buffer[BufferSize] = '\0';
+ State = AsciiStrDecimalToUintn (Buffer);
}
return State;
@@ -129,8 +128,11 @@ XenBusAddDevice (
if (XenStorePathExists (XST_NIL, DevicePath, "")) {
XENBUS_PRIVATE_DATA *Child;
- enum xenbus_state State;
- CHAR8 *BackendPath;
+ enum xenbus_state State;
+ CHAR8 BackendPath[XENSTORE_ABS_PATH_MAX + 1];
+ UINTN BackendPathSize;
+
+ BackendPathSize = sizeof (BackendPath);
Child = XenBusDeviceInitialized (Dev, DevicePath);
if (Child != NULL) {
@@ -155,17 +157,18 @@ XenBusAddDevice (
}
StatusXenStore = XenStoreRead (XST_NIL, DevicePath, "backend",
- NULL, (VOID **) &BackendPath);
+ &BackendPathSize, BackendPath);
if (StatusXenStore != XENSTORE_STATUS_SUCCESS) {
DEBUG ((EFI_D_ERROR, "xenbus: %a no backend path.\n", DevicePath));
Status = EFI_NOT_FOUND;
goto out;
}
+ BackendPath[BackendPathSize] = '\0';
Private = AllocateCopyPool (sizeof (*Private), &gXenBusPrivateData);
Private->XenBusIo.Type = AsciiStrDup (Type);
Private->XenBusIo.Node = AsciiStrDup (DevicePath);
- Private->XenBusIo.Backend = BackendPath;
+ Private->XenBusIo.Backend = AsciiStrDup (BackendPath);
Private->XenBusIo.DeviceId = (UINT16)AsciiStrDecimalToUintn (Id);
Private->Dev = Dev;
@@ -309,17 +312,20 @@ XenBusSetState (
)
{
enum xenbus_state CurrentState;
- XENSTORE_STATUS Status;
- CHAR8 *Temp;
+ XENSTORE_STATUS Status;
+ CHAR8 Buffer[4];
+ UINTN BufferSize;
+
+ BufferSize = sizeof (Buffer) - 1;
DEBUG ((EFI_D_INFO, "XenBus: Set state to %d\n", NewState));
- Status = XenStoreRead (Transaction, This->Node, "state", NULL, (VOID **)&Temp);
+ Status = XenStoreRead (Transaction, This->Node, "state", &BufferSize, Buffer);
if (Status != XENSTORE_STATUS_SUCCESS) {
goto Out;
}
- CurrentState = AsciiStrDecimalToUintn (Temp);
- FreePool (Temp);
+ Buffer[BufferSize] = '\0';
+ CurrentState = AsciiStrDecimalToUintn (Buffer);
if (CurrentState == NewState) {
goto Out;
}
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 004d3b6022..b9588bb8c6 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -756,8 +756,9 @@ XenStoreGetError (
@param RequestType The type of message to send.
@param WriteRequest Pointers to the body sections of the request.
@param NumRequests The number of body sections in the request.
- @param LenPtr The returned length of the reply.
- @param ResultPtr The returned body of the reply.
+ @param BufferSize IN: size of the buffer
+ OUT: The returned length of the reply.
+ @param Buffer The returned body of the reply.
@return XENSTORE_STATUS_SUCCESS on success. Otherwise an errno indicating
the cause of failure.
@@ -769,15 +770,13 @@ XenStoreTalkv (
IN enum xsd_sockmsg_type RequestType,
IN CONST WRITE_REQUEST *WriteRequest,
IN UINT32 NumRequests,
- OUT UINT32 *LenPtr OPTIONAL,
- OUT VOID **ResultPtr OPTIONAL
+ IN OUT UINTN *BufferSize OPTIONAL,
+ OUT VOID *Buffer OPTIONAL
)
{
struct xsd_sockmsg Message;
UINTN Index;
XENSTORE_STATUS Status;
- VOID *Buffer;
- UINTN BufferSize;
if (Transaction == XST_NIL) {
Message.tx_id = 0;
@@ -805,32 +804,15 @@ XenStoreTalkv (
}
}
- if (ResultPtr) {
- Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
- BufferSize = XENSTORE_PAYLOAD_MAX;
- } else {
- Buffer = NULL;
- BufferSize = 0;
- }
-
//
// Wait for a reply to our request
//
Status = XenStoreProcessMessage (Message.req_id, Message.tx_id,
- &BufferSize, Buffer);
+ BufferSize, Buffer);
if (Status != XENSTORE_STATUS_SUCCESS) {
DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
Status));
- FreePool (Buffer);
- return Status;
- }
-
- if (ResultPtr) {
- *ResultPtr = Buffer;
- if (LenPtr) {
- *LenPtr = BufferSize;
- }
}
Error:
@@ -848,8 +830,9 @@ XenStoreTalkv (
@param RequestType The type of message to send.
@param Body The body of the request.
@param SubPath If !NULL and not "", "/$SubPath" is append to Body.
- @param LenPtr The returned length of the reply.
- @param Result The returned body of the reply.
+ @param BufferSize IN: sizef of the buffer
+ OUT: The returned length of the reply.
+ @param Buffer The returned body of the reply.
@return 0 on success. Otherwise an errno indicating
the cause of failure.
@@ -861,8 +844,8 @@ XenStoreSingle (
IN enum xsd_sockmsg_type RequestType,
IN CONST CHAR8 *Body,
IN CONST CHAR8 *SubPath OPTIONAL,
- OUT UINT32 *LenPtr OPTIONAL,
- OUT VOID **Result OPTIONAL
+ IN OUT UINTN *BufferSize OPTIONAL,
+ OUT VOID *Buffer OPTIONAL
)
{
WRITE_REQUEST WriteRequest[3];
@@ -870,7 +853,7 @@ XenStoreSingle (
XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
- LenPtr, Result);
+ BufferSize, Buffer);
}
//
@@ -1106,13 +1089,16 @@ XenStoreListDirectory (
OUT CONST CHAR8 ***DirectoryListPtr
)
{
- CHAR8 *TempStr;
- UINT32 Len = 0;
+ CHAR8 *TempStr;
+ UINTN Len;
XENSTORE_STATUS Status;
+ TempStr = AllocatePool (XENSTORE_PAYLOAD_MAX);
+ Len = XENSTORE_PAYLOAD_MAX;
Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
- (VOID **) &TempStr);
+ TempStr);
if (Status != XENSTORE_STATUS_SUCCESS) {
+ FreePool (TempStr);
return Status;
}
@@ -1146,21 +1132,14 @@ XenStoreRead (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
IN CONST CHAR8 *Node,
- OUT UINT32 *LenPtr OPTIONAL,
- OUT VOID **Result
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
)
{
- VOID *Value;
- XENSTORE_STATUS Status;
-
- Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
- LenPtr, &Value);
- if (Status != XENSTORE_STATUS_SUCCESS) {
- return Status;
- }
-
- *Result = Value;
- return XENSTORE_STATUS_SUCCESS;
+ ASSERT (BufferSize != NULL);
+ ASSERT (Buffer != NULL);
+ return XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
+ BufferSize, Buffer);
}
XENSTORE_STATUS
@@ -1199,14 +1178,16 @@ XenStoreTransactionStart (
OUT XENSTORE_TRANSACTION *Transaction
)
{
- CHAR8 *IdStr;
+ CHAR8 IdStr[XENSTORE_PAYLOAD_MAX];
+ UINTN BufferSize;
XENSTORE_STATUS Status;
+ BufferSize = sizeof (IdStr);
+
Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
- NULL, (VOID **) &IdStr);
+ &BufferSize, IdStr);
if (Status == XENSTORE_STATUS_SUCCESS) {
Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
- FreePool (IdStr);
}
return Status;
@@ -1358,7 +1339,24 @@ XenBusXenStoreRead (
OUT VOID **Value
)
{
- return XenStoreRead (Transaction, This->Node, Node, NULL, Value);
+ XENSTORE_STATUS Status;
+ UINTN BufferSize;
+ VOID *Buffer;
+
+ BufferSize = XENSTORE_PAYLOAD_MAX + 1;
+ Buffer = AllocatePool (BufferSize);
+ if (Buffer == NULL) {
+ return XENSTORE_STATUS_ENOMEM;
+ }
+
+ Status = XenStoreRead (Transaction, This->Node, Node, &BufferSize, Buffer);
+
+ if (Status == XENSTORE_STATUS_SUCCESS) {
+ *Value = Buffer;
+ } else {
+ FreePool (Buffer);
+ }
+ return Status;
}
XENSTORE_STATUS
@@ -1370,7 +1368,24 @@ XenBusXenStoreBackendRead (
OUT VOID **Value
)
{
- return XenStoreRead (Transaction, This->Backend, Node, NULL, Value);
+ XENSTORE_STATUS Status;
+ UINTN BufferSize;
+ VOID *Buffer;
+
+ BufferSize = XENSTORE_PAYLOAD_MAX + 1;
+ Buffer = AllocatePool (BufferSize);
+ if (Buffer == NULL) {
+ return XENSTORE_STATUS_ENOMEM;
+ }
+
+ Status = XenStoreRead (Transaction, This->Backend, Node, &BufferSize, Buffer);
+
+ if (Status == XENSTORE_STATUS_SUCCESS) {
+ *Value = Buffer;
+ } else {
+ FreePool (Buffer);
+ }
+ return Status;
}
XENSTORE_STATUS
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index effaad7336..13f7d132e6 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -64,29 +64,26 @@ XenStorePathExists (
);
/**
- Get the contents of a single "file". Returns the contents in *Result which
- should be freed after use. The length of the value in bytes is returned in
- *LenPtr.
+ Get the contents of a single "file". Copy the contents in Buffer if
+ provided. The length of the value in bytes is returned in *BufferSize.
@param Transaction The XenStore transaction covering this request.
@param DirectoryPath The dirname of the file to read.
@param Node The basename of the file to read.
- @param LenPtr The amount of data read.
- @param Result The returned contents from this file.
+ @param BufferSize IN: size of the buffer
+ OUT: The returned length of the reply.
+ @param Buffer The returned body of the reply.
@return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
indicating the type of failure.
-
- @note The results buffer is malloced and should be free'd by the
- caller.
**/
XENSTORE_STATUS
XenStoreRead (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
IN CONST CHAR8 *Node,
- OUT UINT32 *LenPtr OPTIONAL,
- OUT VOID **Result
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
);
/**
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions
2019-09-13 14:50 ` [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions Anthony PERARD
@ 2019-09-16 16:11 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 16:11 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> We will use a buffer on the stack instead of allocating memory for
> internal functions that are expecting a reply from xenstore.
>
> The external interface XENBUS_PROTOCOL isn't changed yet, so
> allocation are made for XsRead and XsBackendRead.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/XenBusDxe/XenBus.c | 40 ++++++------
> OvmfPkg/XenBusDxe/XenStore.c | 115 ++++++++++++++++++++---------------
> OvmfPkg/XenBusDxe/XenStore.h | 17 +++---
> 3 files changed, 95 insertions(+), 77 deletions(-)
>
Quoting out of order:
> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> index effaad7336..13f7d132e6 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.h
> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> @@ -64,29 +64,26 @@ XenStorePathExists (
> );
>
> /**
> - Get the contents of a single "file". Returns the contents in *Result which
> - should be freed after use. The length of the value in bytes is returned in
> - *LenPtr.
> + Get the contents of a single "file". Copy the contents in Buffer if
> + provided. The length of the value in bytes is returned in *BufferSize.
>
> @param Transaction The XenStore transaction covering this request.
> @param DirectoryPath The dirname of the file to read.
> @param Node The basename of the file to read.
> - @param LenPtr The amount of data read.
> - @param Result The returned contents from this file.
> + @param BufferSize IN: size of the buffer
> + OUT: The returned length of the reply.
> + @param Buffer The returned body of the reply.
>
> @return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
> indicating the type of failure.
(1) I assume this -- i.e. error returned -- covers the case when
BufferSize was too small on input. Can you document that? (Or should it
be obvious from elsewhere?)
> -
> - @note The results buffer is malloced and should be free'd by the
> - caller.
> **/
> XENSTORE_STATUS
> XenStoreRead (
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *DirectoryPath,
> IN CONST CHAR8 *Node,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID **Result
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> );
>
> /**
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 004d3b6022..b9588bb8c6 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -756,8 +756,9 @@ XenStoreGetError (
> @param RequestType The type of message to send.
> @param WriteRequest Pointers to the body sections of the request.
> @param NumRequests The number of body sections in the request.
> - @param LenPtr The returned length of the reply.
> - @param ResultPtr The returned body of the reply.
> + @param BufferSize IN: size of the buffer
> + OUT: The returned length of the reply.
> + @param Buffer The returned body of the reply.
>
> @return XENSTORE_STATUS_SUCCESS on success. Otherwise an errno indicating
> the cause of failure.
> @@ -769,15 +770,13 @@ XenStoreTalkv (
> IN enum xsd_sockmsg_type RequestType,
> IN CONST WRITE_REQUEST *WriteRequest,
> IN UINT32 NumRequests,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID **ResultPtr OPTIONAL
> + IN OUT UINTN *BufferSize OPTIONAL,
> + OUT VOID *Buffer OPTIONAL
> )
> {
> struct xsd_sockmsg Message;
> UINTN Index;
> XENSTORE_STATUS Status;
> - VOID *Buffer;
> - UINTN BufferSize;
>
> if (Transaction == XST_NIL) {
> Message.tx_id = 0;
> @@ -805,32 +804,15 @@ XenStoreTalkv (
> }
> }
>
> - if (ResultPtr) {
> - Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
> - BufferSize = XENSTORE_PAYLOAD_MAX;
> - } else {
> - Buffer = NULL;
> - BufferSize = 0;
> - }
> -
> //
> // Wait for a reply to our request
> //
> Status = XenStoreProcessMessage (Message.req_id, Message.tx_id,
> - &BufferSize, Buffer);
> + BufferSize, Buffer);
(2) Since we're touching this -- please fix the indentation.
>
> if (Status != XENSTORE_STATUS_SUCCESS) {
> DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
> Status));
> - FreePool (Buffer);
> - return Status;
> - }
> -
> - if (ResultPtr) {
> - *ResultPtr = Buffer;
> - if (LenPtr) {
> - *LenPtr = BufferSize;
> - }
> }
>
> Error:
> @@ -848,8 +830,9 @@ XenStoreTalkv (
> @param RequestType The type of message to send.
> @param Body The body of the request.
> @param SubPath If !NULL and not "", "/$SubPath" is append to Body.
> - @param LenPtr The returned length of the reply.
> - @param Result The returned body of the reply.
> + @param BufferSize IN: sizef of the buffer
> + OUT: The returned length of the reply.
> + @param Buffer The returned body of the reply.
>
> @return 0 on success. Otherwise an errno indicating
> the cause of failure.
> @@ -861,8 +844,8 @@ XenStoreSingle (
> IN enum xsd_sockmsg_type RequestType,
> IN CONST CHAR8 *Body,
> IN CONST CHAR8 *SubPath OPTIONAL,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID **Result OPTIONAL
> + IN OUT UINTN *BufferSize OPTIONAL,
> + OUT VOID *Buffer OPTIONAL
> )
> {
> WRITE_REQUEST WriteRequest[3];
> @@ -870,7 +853,7 @@ XenStoreSingle (
> XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
>
> return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
> - LenPtr, Result);
> + BufferSize, Buffer);
(3) Indentation.
> }
>
> //
> @@ -1106,13 +1089,16 @@ XenStoreListDirectory (
> OUT CONST CHAR8 ***DirectoryListPtr
> )
> {
> - CHAR8 *TempStr;
> - UINT32 Len = 0;
> + CHAR8 *TempStr;
> + UINTN Len;
> XENSTORE_STATUS Status;
>
> + TempStr = AllocatePool (XENSTORE_PAYLOAD_MAX);
(4) Should this AllocatePool() be mentioned in the commit message? (And
the others below, too.)
The function calls Split(), as well, and that one allocates memory too.
Are they not used at ExitBooServices()?
> + Len = XENSTORE_PAYLOAD_MAX;
> Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
> - (VOID **) &TempStr);
> + TempStr);
(5) Indentation.
> if (Status != XENSTORE_STATUS_SUCCESS) {
> + FreePool (TempStr);
> return Status;
> }
>
> @@ -1146,21 +1132,14 @@ XenStoreRead (
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *DirectoryPath,
> IN CONST CHAR8 *Node,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID **Result
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> )
> {
> - VOID *Value;
> - XENSTORE_STATUS Status;
> -
> - Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
> - LenPtr, &Value);
> - if (Status != XENSTORE_STATUS_SUCCESS) {
> - return Status;
> - }
> -
> - *Result = Value;
> - return XENSTORE_STATUS_SUCCESS;
> + ASSERT (BufferSize != NULL);
> + ASSERT (Buffer != NULL);
> + return XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
> + BufferSize, Buffer);
> }
>
> XENSTORE_STATUS
> @@ -1199,14 +1178,16 @@ XenStoreTransactionStart (
> OUT XENSTORE_TRANSACTION *Transaction
> )
> {
> - CHAR8 *IdStr;
> + CHAR8 IdStr[XENSTORE_PAYLOAD_MAX];
> + UINTN BufferSize;
> XENSTORE_STATUS Status;
>
> + BufferSize = sizeof (IdStr);
> +
> Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
> - NULL, (VOID **) &IdStr);
> + &BufferSize, IdStr);
(6) Indentation.
> if (Status == XENSTORE_STATUS_SUCCESS) {
> Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
(7) Sorry if I've missed the obvious: what guarantees that IdStr is
NUL-terminated here? In the other functions, we're NUL-terminating
ourselves.
> - FreePool (IdStr);
> }
>
> return Status;
> @@ -1358,7 +1339,24 @@ XenBusXenStoreRead (
> OUT VOID **Value
> )
> {
> - return XenStoreRead (Transaction, This->Node, Node, NULL, Value);
> + XENSTORE_STATUS Status;
> + UINTN BufferSize;
> + VOID *Buffer;
> +
> + BufferSize = XENSTORE_PAYLOAD_MAX + 1;
> + Buffer = AllocatePool (BufferSize);
> + if (Buffer == NULL) {
> + return XENSTORE_STATUS_ENOMEM;
> + }
> +
> + Status = XenStoreRead (Transaction, This->Node, Node, &BufferSize, Buffer);
> +
> + if (Status == XENSTORE_STATUS_SUCCESS) {
> + *Value = Buffer;
> + } else {
> + FreePool (Buffer);
> + }
> + return Status;
> }
>
> XENSTORE_STATUS
> @@ -1370,7 +1368,24 @@ XenBusXenStoreBackendRead (
> OUT VOID **Value
> )
> {
> - return XenStoreRead (Transaction, This->Backend, Node, NULL, Value);
> + XENSTORE_STATUS Status;
> + UINTN BufferSize;
> + VOID *Buffer;
> +
> + BufferSize = XENSTORE_PAYLOAD_MAX + 1;
> + Buffer = AllocatePool (BufferSize);
> + if (Buffer == NULL) {
> + return XENSTORE_STATUS_ENOMEM;
> + }
> +
> + Status = XenStoreRead (Transaction, This->Backend, Node, &BufferSize, Buffer);
> +
> + if (Status == XENSTORE_STATUS_SUCCESS) {
> + *Value = Buffer;
> + } else {
> + FreePool (Buffer);
> + }
> + return Status;
> }
>
> XENSTORE_STATUS
> diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
> index bb8ddbc4d4..78835ec7b3 100644
> --- a/OvmfPkg/XenBusDxe/XenBus.c
> +++ b/OvmfPkg/XenBusDxe/XenBus.c
> @@ -89,19 +89,18 @@ XenBusReadDriverState (
> IN CONST CHAR8 *Path
> )
> {
> - XenbusState State;
> - CHAR8 *Ptr = NULL;
> + XenbusState State;
> + CHAR8 Buffer[4];
> + UINTN BufferSize;
> XENSTORE_STATUS Status;
>
> - Status = XenStoreRead (XST_NIL, Path, "state", NULL, (VOID **)&Ptr);
> + BufferSize = sizeof (Buffer) - 1;
> + Status = XenStoreRead (XST_NIL, Path, "state", &BufferSize, Buffer);
> if (Status != XENSTORE_STATUS_SUCCESS) {
> State = XenbusStateClosed;
> } else {
> - State = AsciiStrDecimalToUintn (Ptr);
> - }
> -
> - if (Ptr != NULL) {
> - FreePool (Ptr);
> + Buffer[BufferSize] = '\0';
> + State = AsciiStrDecimalToUintn (Buffer);
> }
>
> return State;
> @@ -129,8 +128,11 @@ XenBusAddDevice (
>
> if (XenStorePathExists (XST_NIL, DevicePath, "")) {
> XENBUS_PRIVATE_DATA *Child;
> - enum xenbus_state State;
> - CHAR8 *BackendPath;
> + enum xenbus_state State;
> + CHAR8 BackendPath[XENSTORE_ABS_PATH_MAX + 1];
> + UINTN BackendPathSize;
> +
> + BackendPathSize = sizeof (BackendPath);
>
> Child = XenBusDeviceInitialized (Dev, DevicePath);
> if (Child != NULL) {
> @@ -155,17 +157,18 @@ XenBusAddDevice (
> }
>
> StatusXenStore = XenStoreRead (XST_NIL, DevicePath, "backend",
> - NULL, (VOID **) &BackendPath);
> + &BackendPathSize, BackendPath);
(8) Indentation.
> if (StatusXenStore != XENSTORE_STATUS_SUCCESS) {
> DEBUG ((EFI_D_ERROR, "xenbus: %a no backend path.\n", DevicePath));
> Status = EFI_NOT_FOUND;
> goto out;
> }
> + BackendPath[BackendPathSize] = '\0';
(9) I think this could be off-by-one. In the other cases, you decrement
the buffer size first, before passing it to XenStoreRead().
Sorry if I got confused.
>
> Private = AllocateCopyPool (sizeof (*Private), &gXenBusPrivateData);
> Private->XenBusIo.Type = AsciiStrDup (Type);
> Private->XenBusIo.Node = AsciiStrDup (DevicePath);
> - Private->XenBusIo.Backend = BackendPath;
> + Private->XenBusIo.Backend = AsciiStrDup (BackendPath);
(10) Where is this released?
> Private->XenBusIo.DeviceId = (UINT16)AsciiStrDecimalToUintn (Id);
> Private->Dev = Dev;
>
> @@ -309,17 +312,20 @@ XenBusSetState (
> )
> {
> enum xenbus_state CurrentState;
> - XENSTORE_STATUS Status;
> - CHAR8 *Temp;
> + XENSTORE_STATUS Status;
> + CHAR8 Buffer[4];
> + UINTN BufferSize;
> +
> + BufferSize = sizeof (Buffer) - 1;
>
> DEBUG ((EFI_D_INFO, "XenBus: Set state to %d\n", NewState));
>
> - Status = XenStoreRead (Transaction, This->Node, "state", NULL, (VOID **)&Temp);
> + Status = XenStoreRead (Transaction, This->Node, "state", &BufferSize, Buffer);
> if (Status != XENSTORE_STATUS_SUCCESS) {
> goto Out;
> }
> - CurrentState = AsciiStrDecimalToUintn (Temp);
> - FreePool (Temp);
> + Buffer[BufferSize] = '\0';
> + CurrentState = AsciiStrDecimalToUintn (Buffer);
> if (CurrentState == NewState) {
> goto Out;
> }
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (6 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 16:16 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services Anthony PERARD
` (2 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
XsRead and XsBackendRead of the XENBUS_PROTOCOL return allocated
memory but this isn't allowed during the ExitBootServices call. We
need XsRead and XsBackendRead to disconnect from the device so
XENBUS_PROTOCOL is changed to use a buffer supplied by a child driver.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/Include/Protocol/XenBus.h | 32 ++++++++++++----------
OvmfPkg/XenBusDxe/XenStore.c | 44 +++++-------------------------
OvmfPkg/XenBusDxe/XenStore.h | 6 +++--
OvmfPkg/XenPvBlkDxe/BlockFront.c | 45 ++++++++++++++++++-------------
4 files changed, 54 insertions(+), 73 deletions(-)
diff --git a/OvmfPkg/Include/Protocol/XenBus.h b/OvmfPkg/Include/Protocol/XenBus.h
index 8ff5ca3575..c22bdfb368 100644
--- a/OvmfPkg/Include/Protocol/XenBus.h
+++ b/OvmfPkg/Include/Protocol/XenBus.h
@@ -35,6 +35,12 @@ typedef struct
#define XST_NIL ((XENSTORE_TRANSACTION *) NULL)
+//
+// When reading a node from xenstore, if the size of the data to be read is
+// unknown, this value can be use for the size of the buffer.
+//
+#define XENSTORE_PAYLOAD_MAX 4096
+
typedef enum {
XENSTORE_STATUS_SUCCESS = 0,
XENSTORE_STATUS_FAIL,
@@ -64,19 +70,17 @@ typedef enum {
///
/**
- Get the contents of the node Node of the PV device. Returns the contents in
- *Result which should be freed after use.
+ Get the contents of the node Node of the PV device.
@param This A pointer to XENBUS_PROTOCOL instance.
@param Transaction The XenStore transaction covering this request.
@param Node The basename of the file to read.
- @param Result The returned contents from this file.
+ @param BufferSize On input, a pointer to the size of the buffer at Buffer.
+ On output, the size of the data written to Buffer.
+ @param Buffer A pointer to a buffer into which the data read will be saved.
@return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
indicating the type of failure.
-
- @note The results buffer is malloced and should be free'd by the
- caller.
**/
typedef
XENSTORE_STATUS
@@ -84,23 +88,22 @@ XENSTORE_STATUS
IN XENBUS_PROTOCOL *This,
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Node,
- OUT VOID **Result
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
);
/**
- Get the contents of the node Node of the PV device's backend. Returns the
- contents in *Result which should be freed after use.
+ Get the contents of the node Node of the PV device's backend.
@param This A pointer to XENBUS_PROTOCOL instance.
@param Transaction The XenStore transaction covering this request.
@param Node The basename of the file to read.
- @param Result The returned contents from this file.
+ @param BufferSize On input, a pointer to the size of the buffer at Buffer.
+ On output, the size of the data written to Buffer.
+ @param Buffer A pointer to a buffer into which the data read will be saved.
@return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
indicating the type of failure.
-
- @note The results buffer is malloced and should be free'd by the
- caller.
**/
typedef
XENSTORE_STATUS
@@ -108,7 +111,8 @@ XENSTORE_STATUS
IN XENBUS_PROTOCOL *This,
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Node,
- OUT VOID **Result
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
);
/**
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index b9588bb8c6..cb2d9e1215 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1336,27 +1336,11 @@ XenBusXenStoreRead (
IN XENBUS_PROTOCOL *This,
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Node,
- OUT VOID **Value
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
)
{
- XENSTORE_STATUS Status;
- UINTN BufferSize;
- VOID *Buffer;
-
- BufferSize = XENSTORE_PAYLOAD_MAX + 1;
- Buffer = AllocatePool (BufferSize);
- if (Buffer == NULL) {
- return XENSTORE_STATUS_ENOMEM;
- }
-
- Status = XenStoreRead (Transaction, This->Node, Node, &BufferSize, Buffer);
-
- if (Status == XENSTORE_STATUS_SUCCESS) {
- *Value = Buffer;
- } else {
- FreePool (Buffer);
- }
- return Status;
+ return XenStoreRead (Transaction, This->Node, Node, BufferSize, Buffer);
}
XENSTORE_STATUS
@@ -1365,27 +1349,11 @@ XenBusXenStoreBackendRead (
IN XENBUS_PROTOCOL *This,
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Node,
- OUT VOID **Value
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
)
{
- XENSTORE_STATUS Status;
- UINTN BufferSize;
- VOID *Buffer;
-
- BufferSize = XENSTORE_PAYLOAD_MAX + 1;
- Buffer = AllocatePool (BufferSize);
- if (Buffer == NULL) {
- return XENSTORE_STATUS_ENOMEM;
- }
-
- Status = XenStoreRead (Transaction, This->Backend, Node, &BufferSize, Buffer);
-
- if (Status == XENSTORE_STATUS_SUCCESS) {
- *Value = Buffer;
- } else {
- FreePool (Buffer);
- }
- return Status;
+ return XenStoreRead (Transaction, This->Backend, Node, BufferSize, Buffer);
}
XENSTORE_STATUS
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index 13f7d132e6..ca8c080433 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -289,7 +289,8 @@ XenBusXenStoreRead (
IN XENBUS_PROTOCOL *This,
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Node,
- OUT VOID **Value
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
);
XENSTORE_STATUS
@@ -298,7 +299,8 @@ XenBusXenStoreBackendRead (
IN XENBUS_PROTOCOL *This,
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Node,
- OUT VOID **Value
+ IN OUT UINTN *BufferSize,
+ OUT VOID *Buffer
);
XENSTORE_STATUS
diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c
index 8dca4c82f0..25a398ccc4 100644
--- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
+++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
@@ -41,19 +41,22 @@ XenBusReadUint64 (
)
{
XENSTORE_STATUS Status;
- CHAR8 *Ptr;
+ CHAR8 Buffer[22];
+ UINTN BufferSize;
+
+ BufferSize = sizeof (Buffer) - 1;
if (!FromBackend) {
- Status = This->XsRead (This, XST_NIL, Node, (VOID**)&Ptr);
+ Status = This->XsRead (This, XST_NIL, Node, &BufferSize, Buffer);
} else {
- Status = This->XsBackendRead (This, XST_NIL, Node, (VOID**)&Ptr);
+ Status = This->XsBackendRead (This, XST_NIL, Node, &BufferSize, Buffer);
}
if (Status != XENSTORE_STATUS_SUCCESS) {
return Status;
}
+ Buffer[BufferSize] = '\0';
// AsciiStrDecimalToUint64 will ASSERT if Ptr overflow UINT64.
- *ValuePtr = AsciiStrDecimalToUint64 (Ptr);
- FreePool (Ptr);
+ *ValuePtr = AsciiStrDecimalToUint64 (Buffer);
return Status;
}
@@ -143,43 +146,47 @@ XenPvBlockFrontInitialization (
OUT XEN_BLOCK_FRONT_DEVICE **DevPtr
)
{
- XENSTORE_TRANSACTION Transaction;
- CHAR8 *DeviceType;
- blkif_sring_t *SharedRing;
- XENSTORE_STATUS Status;
+ XENSTORE_TRANSACTION Transaction;
+ CHAR8 Buffer[XENSTORE_PAYLOAD_MAX + 1];
+ UINTN BufferSize;
+ blkif_sring_t *SharedRing;
+ XENSTORE_STATUS Status;
XEN_BLOCK_FRONT_DEVICE *Dev;
- XenbusState State;
- UINT64 Value;
- CHAR8 *Params;
+ XenbusState State;
+ UINT64 Value;
ASSERT (NodeName != NULL);
+ BufferSize = sizeof (Buffer) - 1;
+
Dev = AllocateZeroPool (sizeof (XEN_BLOCK_FRONT_DEVICE));
Dev->Signature = XEN_BLOCK_FRONT_SIGNATURE;
Dev->NodeName = NodeName;
Dev->XenBusIo = XenBusIo;
Dev->DeviceId = XenBusIo->DeviceId;
- XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", (VOID**)&DeviceType);
- if (AsciiStrCmp (DeviceType, "cdrom") == 0) {
+ BufferSize = sizeof (Buffer) - 1;
+ XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", &BufferSize, Buffer);
+ Buffer[BufferSize] = '\0';
+ if (AsciiStrCmp (Buffer, "cdrom") == 0) {
Dev->MediaInfo.CdRom = TRUE;
} else {
Dev->MediaInfo.CdRom = FALSE;
}
- FreePool (DeviceType);
if (Dev->MediaInfo.CdRom) {
- Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", (VOID**)&Params);
+ BufferSize = sizeof (Buffer) - 1;
+ Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params",
+ &BufferSize, Buffer);
if (Status != XENSTORE_STATUS_SUCCESS) {
DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n", __FUNCTION__, Status));
goto Error;
}
- if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {
- FreePool (Params);
+ Buffer[BufferSize] = '\0';
+ if (AsciiStrLen (Buffer) == 0 || AsciiStrCmp (Buffer, "aio:") == 0) {
DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__));
goto Error;
}
- FreePool (Params);
}
Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, &Value);
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory
2019-09-13 14:50 ` [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory Anthony PERARD
@ 2019-09-16 16:16 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 16:16 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> XsRead and XsBackendRead of the XENBUS_PROTOCOL return allocated
> memory but this isn't allowed during the ExitBootServices call. We
> need XsRead and XsBackendRead to disconnect from the device so
> XENBUS_PROTOCOL is changed to use a buffer supplied by a child driver.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> OvmfPkg/Include/Protocol/XenBus.h | 32 ++++++++++++----------
> OvmfPkg/XenBusDxe/XenStore.c | 44 +++++-------------------------
> OvmfPkg/XenBusDxe/XenStore.h | 6 +++--
> OvmfPkg/XenPvBlkDxe/BlockFront.c | 45 ++++++++++++++++++-------------
> 4 files changed, 54 insertions(+), 73 deletions(-)
>
> diff --git a/OvmfPkg/Include/Protocol/XenBus.h b/OvmfPkg/Include/Protocol/XenBus.h
> index 8ff5ca3575..c22bdfb368 100644
> --- a/OvmfPkg/Include/Protocol/XenBus.h
> +++ b/OvmfPkg/Include/Protocol/XenBus.h
> @@ -35,6 +35,12 @@ typedef struct
>
> #define XST_NIL ((XENSTORE_TRANSACTION *) NULL)
>
> +//
> +// When reading a node from xenstore, if the size of the data to be read is
> +// unknown, this value can be use for the size of the buffer.
> +//
> +#define XENSTORE_PAYLOAD_MAX 4096
> +
This macro is already defined in "IndustryStandard/Xen/io/xs_wire.h".
Can we get it from there?
The extra documentation is OK, of course (replacing "this value" with
"XENSTORE_PAYLOAD_MAX").
Other than that, I'm going to have to ACK this after a brief skim only.
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> typedef enum {
> XENSTORE_STATUS_SUCCESS = 0,
> XENSTORE_STATUS_FAIL,
> @@ -64,19 +70,17 @@ typedef enum {
> ///
>
> /**
> - Get the contents of the node Node of the PV device. Returns the contents in
> - *Result which should be freed after use.
> + Get the contents of the node Node of the PV device.
>
> @param This A pointer to XENBUS_PROTOCOL instance.
> @param Transaction The XenStore transaction covering this request.
> @param Node The basename of the file to read.
> - @param Result The returned contents from this file.
> + @param BufferSize On input, a pointer to the size of the buffer at Buffer.
> + On output, the size of the data written to Buffer.
> + @param Buffer A pointer to a buffer into which the data read will be saved.
>
> @return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
> indicating the type of failure.
> -
> - @note The results buffer is malloced and should be free'd by the
> - caller.
> **/
> typedef
> XENSTORE_STATUS
> @@ -84,23 +88,22 @@ XENSTORE_STATUS
> IN XENBUS_PROTOCOL *This,
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *Node,
> - OUT VOID **Result
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> );
>
> /**
> - Get the contents of the node Node of the PV device's backend. Returns the
> - contents in *Result which should be freed after use.
> + Get the contents of the node Node of the PV device's backend.
>
> @param This A pointer to XENBUS_PROTOCOL instance.
> @param Transaction The XenStore transaction covering this request.
> @param Node The basename of the file to read.
> - @param Result The returned contents from this file.
> + @param BufferSize On input, a pointer to the size of the buffer at Buffer.
> + On output, the size of the data written to Buffer.
> + @param Buffer A pointer to a buffer into which the data read will be saved.
>
> @return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
> indicating the type of failure.
> -
> - @note The results buffer is malloced and should be free'd by the
> - caller.
> **/
> typedef
> XENSTORE_STATUS
> @@ -108,7 +111,8 @@ XENSTORE_STATUS
> IN XENBUS_PROTOCOL *This,
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *Node,
> - OUT VOID **Result
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> );
>
> /**
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index b9588bb8c6..cb2d9e1215 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1336,27 +1336,11 @@ XenBusXenStoreRead (
> IN XENBUS_PROTOCOL *This,
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *Node,
> - OUT VOID **Value
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> )
> {
> - XENSTORE_STATUS Status;
> - UINTN BufferSize;
> - VOID *Buffer;
> -
> - BufferSize = XENSTORE_PAYLOAD_MAX + 1;
> - Buffer = AllocatePool (BufferSize);
> - if (Buffer == NULL) {
> - return XENSTORE_STATUS_ENOMEM;
> - }
> -
> - Status = XenStoreRead (Transaction, This->Node, Node, &BufferSize, Buffer);
> -
> - if (Status == XENSTORE_STATUS_SUCCESS) {
> - *Value = Buffer;
> - } else {
> - FreePool (Buffer);
> - }
> - return Status;
> + return XenStoreRead (Transaction, This->Node, Node, BufferSize, Buffer);
> }
>
> XENSTORE_STATUS
> @@ -1365,27 +1349,11 @@ XenBusXenStoreBackendRead (
> IN XENBUS_PROTOCOL *This,
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *Node,
> - OUT VOID **Value
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> )
> {
> - XENSTORE_STATUS Status;
> - UINTN BufferSize;
> - VOID *Buffer;
> -
> - BufferSize = XENSTORE_PAYLOAD_MAX + 1;
> - Buffer = AllocatePool (BufferSize);
> - if (Buffer == NULL) {
> - return XENSTORE_STATUS_ENOMEM;
> - }
> -
> - Status = XenStoreRead (Transaction, This->Backend, Node, &BufferSize, Buffer);
> -
> - if (Status == XENSTORE_STATUS_SUCCESS) {
> - *Value = Buffer;
> - } else {
> - FreePool (Buffer);
> - }
> - return Status;
> + return XenStoreRead (Transaction, This->Backend, Node, BufferSize, Buffer);
> }
>
> XENSTORE_STATUS
> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> index 13f7d132e6..ca8c080433 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.h
> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> @@ -289,7 +289,8 @@ XenBusXenStoreRead (
> IN XENBUS_PROTOCOL *This,
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *Node,
> - OUT VOID **Value
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> );
>
> XENSTORE_STATUS
> @@ -298,7 +299,8 @@ XenBusXenStoreBackendRead (
> IN XENBUS_PROTOCOL *This,
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *Node,
> - OUT VOID **Value
> + IN OUT UINTN *BufferSize,
> + OUT VOID *Buffer
> );
>
> XENSTORE_STATUS
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> index 8dca4c82f0..25a398ccc4 100644
> --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> @@ -41,19 +41,22 @@ XenBusReadUint64 (
> )
> {
> XENSTORE_STATUS Status;
> - CHAR8 *Ptr;
> + CHAR8 Buffer[22];
> + UINTN BufferSize;
> +
> + BufferSize = sizeof (Buffer) - 1;
>
> if (!FromBackend) {
> - Status = This->XsRead (This, XST_NIL, Node, (VOID**)&Ptr);
> + Status = This->XsRead (This, XST_NIL, Node, &BufferSize, Buffer);
> } else {
> - Status = This->XsBackendRead (This, XST_NIL, Node, (VOID**)&Ptr);
> + Status = This->XsBackendRead (This, XST_NIL, Node, &BufferSize, Buffer);
> }
> if (Status != XENSTORE_STATUS_SUCCESS) {
> return Status;
> }
> + Buffer[BufferSize] = '\0';
> // AsciiStrDecimalToUint64 will ASSERT if Ptr overflow UINT64.
> - *ValuePtr = AsciiStrDecimalToUint64 (Ptr);
> - FreePool (Ptr);
> + *ValuePtr = AsciiStrDecimalToUint64 (Buffer);
> return Status;
> }
>
> @@ -143,43 +146,47 @@ XenPvBlockFrontInitialization (
> OUT XEN_BLOCK_FRONT_DEVICE **DevPtr
> )
> {
> - XENSTORE_TRANSACTION Transaction;
> - CHAR8 *DeviceType;
> - blkif_sring_t *SharedRing;
> - XENSTORE_STATUS Status;
> + XENSTORE_TRANSACTION Transaction;
> + CHAR8 Buffer[XENSTORE_PAYLOAD_MAX + 1];
> + UINTN BufferSize;
> + blkif_sring_t *SharedRing;
> + XENSTORE_STATUS Status;
> XEN_BLOCK_FRONT_DEVICE *Dev;
> - XenbusState State;
> - UINT64 Value;
> - CHAR8 *Params;
> + XenbusState State;
> + UINT64 Value;
>
> ASSERT (NodeName != NULL);
>
> + BufferSize = sizeof (Buffer) - 1;
> +
> Dev = AllocateZeroPool (sizeof (XEN_BLOCK_FRONT_DEVICE));
> Dev->Signature = XEN_BLOCK_FRONT_SIGNATURE;
> Dev->NodeName = NodeName;
> Dev->XenBusIo = XenBusIo;
> Dev->DeviceId = XenBusIo->DeviceId;
>
> - XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", (VOID**)&DeviceType);
> - if (AsciiStrCmp (DeviceType, "cdrom") == 0) {
> + BufferSize = sizeof (Buffer) - 1;
> + XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", &BufferSize, Buffer);
> + Buffer[BufferSize] = '\0';
> + if (AsciiStrCmp (Buffer, "cdrom") == 0) {
> Dev->MediaInfo.CdRom = TRUE;
> } else {
> Dev->MediaInfo.CdRom = FALSE;
> }
> - FreePool (DeviceType);
>
> if (Dev->MediaInfo.CdRom) {
> - Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", (VOID**)&Params);
> + BufferSize = sizeof (Buffer) - 1;
> + Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params",
> + &BufferSize, Buffer);
> if (Status != XENSTORE_STATUS_SUCCESS) {
> DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n", __FUNCTION__, Status));
> goto Error;
> }
> - if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {
> - FreePool (Params);
> + Buffer[BufferSize] = '\0';
> + if (AsciiStrLen (Buffer) == 0 || AsciiStrCmp (Buffer, "aio:") == 0) {
> DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__));
> goto Error;
> }
> - FreePool (Params);
> }
>
> Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, &Value);
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (7 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-16 17:36 ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 10/11] OvmfPkg/XenPvBlkDxe: Use XenBusIo->RegisterExitCallback Anthony PERARD
2019-09-13 14:51 ` [PATCH 11/11] OvmfPkg/XenBusDxe: Fix XenStoreWaitForEvent use during EBS Anthony PERARD
10 siblings, 1 reply; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoid
using the Memory Allocation Services.
This comes with a new interface named RegisterExitCallback so that PV
drivers can disconnect from the backend before XenBusDxe is teared
down.
Instead of using Disconnect() to tear down the XenBus driver and the
children drivers, we are going to ask every driver using
XENBUS_PROTOCOL to disconnect from the hardware via the callback set
with RegisterExitCallback, then reset the xenstore shared ring and
the grant table.
Since there are no driver using RegisterExitCallback yet, no driver are
going to be disconnected. Linux can deal with that. And that will be
fixed by the next patch with a change for XenPvBlkDxe.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/Include/Protocol/XenBus.h | 35 +++++++++++++++++++++++++++++++
OvmfPkg/XenBusDxe/XenBus.c | 18 ++++++++++++++++
OvmfPkg/XenBusDxe/XenBusDxe.c | 27 +++++++++++++++++++++---
OvmfPkg/XenBusDxe/XenBusDxe.h | 2 ++
OvmfPkg/XenBusDxe/XenStore.c | 22 ++++++++++++++++++-
OvmfPkg/XenBusDxe/XenStore.h | 21 +++++++++++++++++++
6 files changed, 121 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/Include/Protocol/XenBus.h b/OvmfPkg/Include/Protocol/XenBus.h
index c22bdfb368..04986747c8 100644
--- a/OvmfPkg/Include/Protocol/XenBus.h
+++ b/OvmfPkg/Include/Protocol/XenBus.h
@@ -373,6 +373,38 @@ XENSTORE_STATUS
IN VOID *Token
);
+typedef
+VOID
+(EFIAPI *XENBUS_EXIT_CALLBACK)(
+ IN VOID *Context
+ );
+
+/**
+ Register a function to be called during the ExitBootServices event.
+
+ NotifyFunction will be called when XenBusDxe is notified of
+ EVT_SIGNAL_EXIT_BOOT_SERVICES. The function should follow the same
+ requirements as if it as registered an event on
+ EVT_SIGNAL_EXIT_BOOT_SERVICES, i.e. no use of the Memory Allocation
+ Services.
+
+ To unregister the function, call RegisterExitCallback with
+ NotifyFunction=NULL.
+
+ @note There can only be one callback per driver.
+
+ @param This A pointer to the XENBUS_PROTOCOL.
+ @param NotifyFunction The function to be called.
+ @param NotifyContext A context.
+**/
+typedef
+VOID
+(EFIAPI *XENBUS_SET_EXIT_CALLBACK) (
+ IN XENBUS_PROTOCOL *This,
+ IN XENBUS_EXIT_CALLBACK NotifyFunction,
+ IN VOID *NotifyContext
+ );
+
///
/// Protocol structure
@@ -400,6 +432,9 @@ struct _XENBUS_PROTOCOL {
XENBUS_REGISTER_WATCH_BACKEND RegisterWatchBackend;
XENBUS_UNREGISTER_WATCH UnregisterWatch;
XENBUS_WAIT_FOR_WATCH WaitForWatch;
+
+ XENBUS_SET_EXIT_CALLBACK RegisterExitCallback;
+
//
// Protocol data fields
//
diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
index 78835ec7b3..54166505d2 100644
--- a/OvmfPkg/XenBusDxe/XenBus.c
+++ b/OvmfPkg/XenBusDxe/XenBus.c
@@ -343,6 +343,23 @@ XenBusSetState (
return Status;
}
+STATIC
+VOID
+EFIAPI
+XenBusRegisterExitCallback (
+ IN XENBUS_PROTOCOL *This,
+ IN XENBUS_EXIT_CALLBACK NotifyFunction,
+ IN VOID *NotifyContext
+ )
+{
+ XENBUS_PRIVATE_DATA *Dev;
+
+ Dev = XENBUS_PRIVATE_DATA_FROM_THIS (This);
+
+ Dev->ExitCallback = NotifyFunction;
+ Dev->ExitCallbackContext = NotifyContext;
+}
+
STATIC XENBUS_PRIVATE_DATA gXenBusPrivateData = {
XENBUS_PRIVATE_DATA_SIGNATURE, // Signature
{ NULL, NULL }, // Link
@@ -364,6 +381,7 @@ STATIC XENBUS_PRIVATE_DATA gXenBusPrivateData = {
XenBusRegisterWatchBackend, // XenBusIo.RegisterWatchBackend
XenBusUnregisterWatch, // XenBusIo.UnregisterWatch
XenBusWaitForWatch, // XenBusIo.WaitForWatch
+ XenBusRegisterExitCallback, // XenBusIo.RegisterExitCallback
NULL, // XenBusIo.Type
0, // XenBusIo.DeviceId
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
index 634c7b71eb..c71966a666 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.c
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
@@ -258,10 +258,31 @@ NotifyExitBoot (
IN VOID *Context
)
{
- XENBUS_DEVICE *Dev = Context;
+ XENBUS_DEVICE *Dev;
+ LIST_ENTRY *Entry;
+ XENBUS_PRIVATE_DATA *Child;
- gBS->DisconnectController(Dev->ControllerHandle,
- Dev->This->DriverBindingHandle, NULL);
+ Dev = Context;
+
+ //
+ // First, ask every driver using xenbus to disconnect without
+ // deallocating memory.
+ //
+ for (Entry = GetFirstNode (&Dev->ChildList);
+ !IsNodeAtEnd (&Dev->ChildList, Entry);
+ Entry = GetNextNode (&Dev->ChildList, Entry)) {
+ Child = XENBUS_PRIVATE_DATA_FROM_LINK (Entry);
+ if (Child->ExitCallback != NULL) {
+ Child->ExitCallback (Child->ExitCallbackContext);
+ }
+ }
+
+ //
+ // Now, we can reset the devices used by XenBusDxe.
+ //
+ XenStoreResetWatches ();
+ XenStoreResetRing (Dev);
+ XenGrantTableDeinit (Dev);
}
/**
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
index b1dcc3549c..0e91c24338 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.h
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
@@ -94,6 +94,8 @@ typedef struct {
XENBUS_PROTOCOL XenBusIo;
XENBUS_DEVICE *Dev;
XENBUS_DEVICE_PATH *DevicePath;
+ XENBUS_EXIT_CALLBACK ExitCallback;
+ VOID *ExitCallbackContext;
} XENBUS_PRIVATE_DATA;
#define XENBUS_PRIVATE_DATA_FROM_THIS(a) \
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index cb2d9e1215..4026c8a829 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1054,8 +1054,18 @@ XenStoreDeinit (
}
}
+ XenStoreResetRing (Dev);
+
gBS->CloseEvent (xs.EventChannelEvent);
+ xs.XenStore = NULL;
+}
+
+VOID
+XenStoreResetRing (
+ IN XENBUS_DEVICE *Dev
+ )
+{
if (xs.XenStore->server_features & XENSTORE_SERVER_FEATURE_RECONNECTION) {
xs.XenStore->connection = XENSTORE_RECONNECT;
XenEventChannelNotify (xs.Dev, xs.EventChannel);
@@ -1072,7 +1082,17 @@ XenStoreDeinit (
xs.XenStore->req_cons = xs.XenStore->req_prod = 0;
xs.XenStore->rsp_cons = xs.XenStore->rsp_prod = 0;
}
- xs.XenStore = NULL;
+}
+
+VOID
+XenStoreResetWatches (
+ VOID
+ )
+{
+ XENSTORE_STATUS Status;
+
+ Status = XenStoreSingle (XST_NIL, XS_RESET_WATCHES, "", NULL, NULL, NULL);
+ ASSERT (Status == XENSTORE_STATUS_SUCCESS);
}
//
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index ca8c080433..62cd5e97bf 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -271,6 +271,27 @@ XenStoreDeinit (
IN XENBUS_DEVICE *Dev
);
+/**
+ Effectively reset all watches registered with xenstore.
+
+ To be used by ExitBootServices
+**/
+VOID
+XenStoreResetWatches (
+ VOID
+ );
+
+/**
+ Reset the xenstore shared ring before handing it over to the next
+ operating system.
+
+ To be used by ExitBootServices
+**/
+VOID
+XenStoreResetRing (
+ IN XENBUS_DEVICE *Dev
+ );
+
//
// XENBUS protocol
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services
2019-09-13 14:50 ` [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services Anthony PERARD
@ 2019-09-16 17:36 ` Laszlo Ersek
2019-09-16 18:36 ` Andrew Fish
0 siblings, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 17:36 UTC (permalink / raw)
To: devel, anthony.perard
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel
On 09/13/19 16:50, Anthony PERARD wrote:
> This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoid
> using the Memory Allocation Services.
>
> This comes with a new interface named RegisterExitCallback so that PV
> drivers can disconnect from the backend before XenBusDxe is teared
> down.
>
> Instead of using Disconnect() to tear down the XenBus driver and the
> children drivers, we are going to ask every driver using
> XENBUS_PROTOCOL to disconnect from the hardware via the callback set
> with RegisterExitCallback, then reset the xenstore shared ring and
> the grant table.
I think this approach -- a lower-level bus driver calling out to
dependent device drivers -- is quite unusual.
How about the following instead:
- introduce two XenBusIo protocol member functions, AddReference() and
RemoveReference(). RemoveReference() should take a BOOLEAN called
"HandOffToOs". The device drivers should call AddReference() just before
exiting DriverBindingStart() with success, and RemoveReference(FALSE) in
DriverBindingStop().
- these protocol member functions would increment / decrement a
reference counter in the underlying XenBus abstraction. Additionally,
RemoveReference() would store the HandOffToOs parameter to a bus-level
BOOLEAN too (regardless of previous value stored there -- a TRUE->FALSE
transition would never happen anyway; see below).
- both XenBusDxe and the Xen device drivers should register EBS
callbacks, per controller handle (in BindingStart()), and unregister
them (in BindingStop())
- the ordering between EBS notification functions (queued at the same
TPL) is unspecified. In the device driver notification functions, the
last action should be a call to XenBusIo->RemoveReference(TRUE) -- after
the device-specific "forget me" actions have been done.
- if RemoveReference() gets a TRUE parameter, then it should check the
resultant (post-decrement) value of the refcount. If it has gone to
zero, RemoveReference() should re-set the xenbus / xenstore connection.
If the parameter is FALSE, it shouldn't do anything particular after
decrementing the refcount.
- in the XenBus EBS handler, if the refcount is positive at the time of
the call, nothing should be done. Otherwise, if HandOffToOs is TRUE,
nothing should be done, similarly. Otherwise, the xenbus/xenstore
connection should be re-set.
The idea is that normal Start/Stop should manage the refcount as
expected. At ExitBootServices(), the XenBus level handler should only
clear the connection to the hypervisor if no RemoveReference() call has
done, or will do, it. (If the counter is positive, then a later
RemoveReference() call will do it; if it's zero but HandOffToOs is TRUE,
then it's been done already. If the counter is zero and the BOOLEAN is
FALSE, then all devices have been disconnected normally with Stop() --
or none have been connected at all --, before ExitBootServices(), so the
XenBus driver itself has to ask for being forgotten.)
Admittedly, this is more complicated (due to the unspecified ordering
between EBS notifications). I just feel it's more idiomatic to go
through normal protocol member functions in EBS notification functions,
rather than special callbacks.
(Side comment: the reference counting could normally be replaced by
gBS->OpenProtocolInformation(); however, that service itself allocates
memory, so we can't use it in EBS notification functions.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services
2019-09-16 17:36 ` [edk2-devel] " Laszlo Ersek
@ 2019-09-16 18:36 ` Andrew Fish
2019-09-16 19:31 ` Laszlo Ersek
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Fish @ 2019-09-16 18:36 UTC (permalink / raw)
To: devel, lersek
Cc: Anthony Perard, Ard Biesheuvel, Julien Grall, Jordan Justen,
xen-devel
> On Sep 16, 2019, at 10:36 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 09/13/19 16:50, Anthony PERARD wrote:
>> This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoid
>> using the Memory Allocation Services.
>>
>> This comes with a new interface named RegisterExitCallback so that PV
>> drivers can disconnect from the backend before XenBusDxe is teared
>> down.
>>
>> Instead of using Disconnect() to tear down the XenBus driver and the
>> children drivers, we are going to ask every driver using
>> XENBUS_PROTOCOL to disconnect from the hardware via the callback set
>> with RegisterExitCallback, then reset the xenstore shared ring and
>> the grant table.
>
> I think this approach -- a lower-level bus driver calling out to
> dependent device drivers -- is quite unusual.
>
Laszlo,
I agree given the timer event activity is stopped prior to calling any of the EXIT_BOOT_SERVICES events there is usually not a requirement to call the drivers Stop() function. Generally Exit Boot Services events just turn off DMA, or any other hardware that could touch memory that is being freed back for OS usage. Since the timer activity, and thus all event activity is stopped there is not a lot of ways for the drivers to ever have any EFI code run again.
The only other exception I can think of is if the OS driver makes some kind of assumption about the state of the hardware.
Thanks,
Andrew Fish
> How about the following instead:
>
> - introduce two XenBusIo protocol member functions, AddReference() and
> RemoveReference(). RemoveReference() should take a BOOLEAN called
> "HandOffToOs". The device drivers should call AddReference() just before
> exiting DriverBindingStart() with success, and RemoveReference(FALSE) in
> DriverBindingStop().
>
> - these protocol member functions would increment / decrement a
> reference counter in the underlying XenBus abstraction. Additionally,
> RemoveReference() would store the HandOffToOs parameter to a bus-level
> BOOLEAN too (regardless of previous value stored there -- a TRUE->FALSE
> transition would never happen anyway; see below).
>
> - both XenBusDxe and the Xen device drivers should register EBS
> callbacks, per controller handle (in BindingStart()), and unregister
> them (in BindingStop())
>
> - the ordering between EBS notification functions (queued at the same
> TPL) is unspecified. In the device driver notification functions, the
> last action should be a call to XenBusIo->RemoveReference(TRUE) -- after
> the device-specific "forget me" actions have been done.
>
> - if RemoveReference() gets a TRUE parameter, then it should check the
> resultant (post-decrement) value of the refcount. If it has gone to
> zero, RemoveReference() should re-set the xenbus / xenstore connection.
> If the parameter is FALSE, it shouldn't do anything particular after
> decrementing the refcount.
>
> - in the XenBus EBS handler, if the refcount is positive at the time of
> the call, nothing should be done. Otherwise, if HandOffToOs is TRUE,
> nothing should be done, similarly. Otherwise, the xenbus/xenstore
> connection should be re-set.
>
> The idea is that normal Start/Stop should manage the refcount as
> expected. At ExitBootServices(), the XenBus level handler should only
> clear the connection to the hypervisor if no RemoveReference() call has
> done, or will do, it. (If the counter is positive, then a later
> RemoveReference() call will do it; if it's zero but HandOffToOs is TRUE,
> then it's been done already. If the counter is zero and the BOOLEAN is
> FALSE, then all devices have been disconnected normally with Stop() --
> or none have been connected at all --, before ExitBootServices(), so the
> XenBus driver itself has to ask for being forgotten.)
>
> Admittedly, this is more complicated (due to the unspecified ordering
> between EBS notifications). I just feel it's more idiomatic to go
> through normal protocol member functions in EBS notification functions,
> rather than special callbacks.
>
> (Side comment: the reference counting could normally be replaced by
> gBS->OpenProtocolInformation(); however, that service itself allocates
> memory, so we can't use it in EBS notification functions.)
>
> Thanks
> Laszlo
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services
2019-09-16 18:36 ` Andrew Fish
@ 2019-09-16 19:31 ` Laszlo Ersek
2019-09-16 20:50 ` Andrew Fish
0 siblings, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2019-09-16 19:31 UTC (permalink / raw)
To: Andrew Fish, devel
Cc: Anthony Perard, Ard Biesheuvel, Julien Grall, Jordan Justen,
xen-devel
On 09/16/19 20:36, Andrew Fish wrote:
>
>
>> On Sep 16, 2019, at 10:36 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 09/13/19 16:50, Anthony PERARD wrote:
>>> This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoid
>>> using the Memory Allocation Services.
>>>
>>> This comes with a new interface named RegisterExitCallback so that PV
>>> drivers can disconnect from the backend before XenBusDxe is teared
>>> down.
>>>
>>> Instead of using Disconnect() to tear down the XenBus driver and the
>>> children drivers, we are going to ask every driver using
>>> XENBUS_PROTOCOL to disconnect from the hardware via the callback set
>>> with RegisterExitCallback, then reset the xenstore shared ring and
>>> the grant table.
>>
>> I think this approach -- a lower-level bus driver calling out to
>> dependent device drivers -- is quite unusual.
>>
>
> Laszlo,
>
> I agree given the timer event activity is stopped prior to calling any of the EXIT_BOOT_SERVICES events there is usually not a requirement to call the drivers Stop() function. Generally Exit Boot Services events just turn off DMA, or any other hardware that could touch memory that is being freed back for OS usage. Since the timer activity, and thus all event activity is stopped there is not a lot of ways for the drivers to ever have any EFI code run again.
>
> The only other exception I can think of is if the OS driver makes some kind of assumption about the state of the hardware.
The
hardware that could touch memory that is being freed back for OS
usage
is the part that applies here. Most paravirtual devices work by sharing
at least some memory between the host-side device model (emulation) and
guest-side device driver. When the guest is transitioning from firmware
to OS (and those of course have different guest drivers for the
paravirtual device), the host must be made forget the memory shared with
the guest firmware driver (as the guest OS boot loader or the guest OS
itself might load anything at all into that RAM area after
ExitBootServices()).
So what I found unusual in this patch wasn't the necessity of EBS
notification functions, but how they would be ordered / coordinated
between each other. There is a bus-like device that shares its own
resources (RAM) with the host, and installs protocol interfaces. And
there are dependent endpoint-like devices that consume those protocol
interfaces, and share their own stuff (RAM) with the host OS.
All of that shared memory needs to be cleared from the host's
book-keeping when we leave firmware land; the tricky part is that the
bus-like device can't request the host for its bus-level shared memory
to be forgotten before all of the dependent endpoints ask for their
shared ranges to be forgotten.
Thanks!
Laszlo
>> How about the following instead:
>>
>> - introduce two XenBusIo protocol member functions, AddReference() and
>> RemoveReference(). RemoveReference() should take a BOOLEAN called
>> "HandOffToOs". The device drivers should call AddReference() just before
>> exiting DriverBindingStart() with success, and RemoveReference(FALSE) in
>> DriverBindingStop().
>>
>> - these protocol member functions would increment / decrement a
>> reference counter in the underlying XenBus abstraction. Additionally,
>> RemoveReference() would store the HandOffToOs parameter to a bus-level
>> BOOLEAN too (regardless of previous value stored there -- a TRUE->FALSE
>> transition would never happen anyway; see below).
>>
>> - both XenBusDxe and the Xen device drivers should register EBS
>> callbacks, per controller handle (in BindingStart()), and unregister
>> them (in BindingStop())
>>
>> - the ordering between EBS notification functions (queued at the same
>> TPL) is unspecified. In the device driver notification functions, the
>> last action should be a call to XenBusIo->RemoveReference(TRUE) -- after
>> the device-specific "forget me" actions have been done.
>>
>> - if RemoveReference() gets a TRUE parameter, then it should check the
>> resultant (post-decrement) value of the refcount. If it has gone to
>> zero, RemoveReference() should re-set the xenbus / xenstore connection.
>> If the parameter is FALSE, it shouldn't do anything particular after
>> decrementing the refcount.
>>
>> - in the XenBus EBS handler, if the refcount is positive at the time of
>> the call, nothing should be done. Otherwise, if HandOffToOs is TRUE,
>> nothing should be done, similarly. Otherwise, the xenbus/xenstore
>> connection should be re-set.
>>
>> The idea is that normal Start/Stop should manage the refcount as
>> expected. At ExitBootServices(), the XenBus level handler should only
>> clear the connection to the hypervisor if no RemoveReference() call has
>> done, or will do, it. (If the counter is positive, then a later
>> RemoveReference() call will do it; if it's zero but HandOffToOs is TRUE,
>> then it's been done already. If the counter is zero and the BOOLEAN is
>> FALSE, then all devices have been disconnected normally with Stop() --
>> or none have been connected at all --, before ExitBootServices(), so the
>> XenBus driver itself has to ask for being forgotten.)
>>
>> Admittedly, this is more complicated (due to the unspecified ordering
>> between EBS notifications). I just feel it's more idiomatic to go
>> through normal protocol member functions in EBS notification functions,
>> rather than special callbacks.
>>
>> (Side comment: the reference counting could normally be replaced by
>> gBS->OpenProtocolInformation(); however, that service itself allocates
>> memory, so we can't use it in EBS notification functions.)
>>
>> Thanks
>> Laszlo
>>
>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services
2019-09-16 19:31 ` Laszlo Ersek
@ 2019-09-16 20:50 ` Andrew Fish
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Fish @ 2019-09-16 20:50 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, Anthony Perard, Ard Biesheuvel, Julien Grall,
Jordan Justen, xen-devel
[-- Attachment #1: Type: text/plain, Size: 7708 bytes --]
> On Sep 16, 2019, at 12:31 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 09/16/19 20:36, Andrew Fish wrote:
>>
>>
>>> On Sep 16, 2019, at 10:36 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>> On 09/13/19 16:50, Anthony PERARD wrote:
>>>> This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoid
>>>> using the Memory Allocation Services.
>>>>
>>>> This comes with a new interface named RegisterExitCallback so that PV
>>>> drivers can disconnect from the backend before XenBusDxe is teared
>>>> down.
>>>>
>>>> Instead of using Disconnect() to tear down the XenBus driver and the
>>>> children drivers, we are going to ask every driver using
>>>> XENBUS_PROTOCOL to disconnect from the hardware via the callback set
>>>> with RegisterExitCallback, then reset the xenstore shared ring and
>>>> the grant table.
>>>
>>> I think this approach -- a lower-level bus driver calling out to
>>> dependent device drivers -- is quite unusual.
>>>
>>
>> Laszlo,
>>
>> I agree given the timer event activity is stopped prior to calling any of the EXIT_BOOT_SERVICES events there is usually not a requirement to call the drivers Stop() function. Generally Exit Boot Services events just turn off DMA, or any other hardware that could touch memory that is being freed back for OS usage. Since the timer activity, and thus all event activity is stopped there is not a lot of ways for the drivers to ever have any EFI code run again.
>>
>> The only other exception I can think of is if the OS driver makes some kind of assumption about the state of the hardware.
>
> The
>
> hardware that could touch memory that is being freed back for OS
> usage
>
> is the part that applies here. Most paravirtual devices work by sharing
> at least some memory between the host-side device model (emulation) and
> guest-side device driver. When the guest is transitioning from firmware
> to OS (and those of course have different guest drivers for the
> paravirtual device), the host must be made forget the memory shared with
> the guest firmware driver (as the guest OS boot loader or the guest OS
> itself might load anything at all into that RAM area after
> ExitBootServices()).
>
> So what I found unusual in this patch wasn't the necessity of EBS
> notification functions, but how they would be ordered / coordinated
> between each other. There is a bus-like device that shares its own
> resources (RAM) with the host, and installs protocol interfaces. And
> there are dependent endpoint-like devices that consume those protocol
> interfaces, and share their own stuff (RAM) with the host OS.
>
> All of that shared memory needs to be cleared from the host's
> book-keeping when we leave firmware land; the tricky part is that the
> bus-like device can't request the host for its bus-level shared memory
> to be forgotten before all of the dependent endpoints ask for their
> shared ranges to be forgotten.
>
Laszlo,
In "real hardware" the bus driver can usually clear the memory usage by by turning off the DMA. I guess this would look like sending a reset to the virtual device. The buffers allocated by children of the "hardware" bus driver free all their buffers back to the OS as a side effect of the ExitBootServices, so it is typical for the children of the bus driver to not have an ExitBootServices event.
I find it strange there is not a reset operation for the virtual bus device, but it seems like worst case the bus driver could could track all the shared memory allocations and free the child allocations 1st?
In general the ExitBootServices routines for hardware can be very simple since EFI is a cooperative event model and the events are shutdown as part of the ExitBootService processes. Most of the times I've seen complex code in ExitBootServices routines it was due to people thinking in terms of threading vs. events, and forgetting that just returning from ExitBootServices frees most of the the resources allocated by the driver.
I'd also point out the order of the ExitBootServices events are not architectural, but a given implementation may do the same thing every time. For example the ExitBootServices events are probably just an insertion to a doubly linked list based on execution order of the drivers. But if a lot of the drivers are Driver Model drivers then they all have the same Depex so the order could depend on the order of those drivers in the FV. Some times the root bridge driver is a DXE driver for something like PCI, but a USB Host Bridge driver would be a Driver Model driver that Starts on a PCI IO Handle.
Thanks,
Andrew Fish
> Thanks!
> Laszlo
>
>
>>> How about the following instead:
>>>
>>> - introduce two XenBusIo protocol member functions, AddReference() and
>>> RemoveReference(). RemoveReference() should take a BOOLEAN called
>>> "HandOffToOs". The device drivers should call AddReference() just before
>>> exiting DriverBindingStart() with success, and RemoveReference(FALSE) in
>>> DriverBindingStop().
>>>
>>> - these protocol member functions would increment / decrement a
>>> reference counter in the underlying XenBus abstraction. Additionally,
>>> RemoveReference() would store the HandOffToOs parameter to a bus-level
>>> BOOLEAN too (regardless of previous value stored there -- a TRUE->FALSE
>>> transition would never happen anyway; see below).
>>>
>>> - both XenBusDxe and the Xen device drivers should register EBS
>>> callbacks, per controller handle (in BindingStart()), and unregister
>>> them (in BindingStop())
>>>
>>> - the ordering between EBS notification functions (queued at the same
>>> TPL) is unspecified. In the device driver notification functions, the
>>> last action should be a call to XenBusIo->RemoveReference(TRUE) -- after
>>> the device-specific "forget me" actions have been done.
>>>
>>> - if RemoveReference() gets a TRUE parameter, then it should check the
>>> resultant (post-decrement) value of the refcount. If it has gone to
>>> zero, RemoveReference() should re-set the xenbus / xenstore connection.
>>> If the parameter is FALSE, it shouldn't do anything particular after
>>> decrementing the refcount.
>>>
>>> - in the XenBus EBS handler, if the refcount is positive at the time of
>>> the call, nothing should be done. Otherwise, if HandOffToOs is TRUE,
>>> nothing should be done, similarly. Otherwise, the xenbus/xenstore
>>> connection should be re-set.
>>>
>>> The idea is that normal Start/Stop should manage the refcount as
>>> expected. At ExitBootServices(), the XenBus level handler should only
>>> clear the connection to the hypervisor if no RemoveReference() call has
>>> done, or will do, it. (If the counter is positive, then a later
>>> RemoveReference() call will do it; if it's zero but HandOffToOs is TRUE,
>>> then it's been done already. If the counter is zero and the BOOLEAN is
>>> FALSE, then all devices have been disconnected normally with Stop() --
>>> or none have been connected at all --, before ExitBootServices(), so the
>>> XenBus driver itself has to ask for being forgotten.)
>>>
>>> Admittedly, this is more complicated (due to the unspecified ordering
>>> between EBS notifications). I just feel it's more idiomatic to go
>>> through normal protocol member functions in EBS notification functions,
>>> rather than special callbacks.
>>>
>>> (Side comment: the reference counting could normally be replaced by
>>> gBS->OpenProtocolInformation(); however, that service itself allocates
>>> memory, so we can't use it in EBS notification functions.)
>>>
>>> Thanks
>>> Laszlo
>>>
>>>
[-- Attachment #2: Type: text/html, Size: 30653 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 10/11] OvmfPkg/XenPvBlkDxe: Use XenBusIo->RegisterExitCallback
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (8 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services Anthony PERARD
@ 2019-09-13 14:50 ` Anthony PERARD
2019-09-13 14:51 ` [PATCH 11/11] OvmfPkg/XenBusDxe: Fix XenStoreWaitForEvent use during EBS Anthony PERARD
10 siblings, 0 replies; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:50 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
In order to be able to reset the backend before handing it to the next
operating system, it should be reset properly. This patch register a
callback function to be called by XenBusDxe during the
ExitBootServices event.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenPvBlkDxe/BlockFront.c | 37 ++++++++++++++++++++++++++++---
OvmfPkg/XenPvBlkDxe/BlockFront.h | 12 +++++++++-
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c | 4 ++--
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c
index 25a398ccc4..7c166888bd 100644
--- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
+++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
@@ -309,6 +309,8 @@ XenPvBlockFrontInitialization (
Dev->MediaInfo.Sectors, Dev->MediaInfo.SectorSize));
*DevPtr = Dev;
+
+ XenBusIo->RegisterExitCallback (XenBusIo, XenPvBlockFrontReset, Dev);
return EFI_SUCCESS;
Error2:
@@ -326,13 +328,16 @@ XenPvBlockFrontInitialization (
VOID
XenPvBlockFrontShutdown (
- IN XEN_BLOCK_FRONT_DEVICE *Dev
+ IN XEN_BLOCK_FRONT_DEVICE *Dev,
+ IN BOOLEAN ResetOnly
)
{
XENBUS_PROTOCOL *XenBusIo = Dev->XenBusIo;
XENSTORE_STATUS Status;
UINT64 Value;
+ XenBusIo->RegisterExitCallback (XenBusIo, NULL, NULL);
+
XenPvBlockSync (Dev);
Status = XenBusIo->SetState (XenBusIo, XST_NIL, XenbusStateClosing);
@@ -393,12 +398,38 @@ XenPvBlockFrontShutdown (
}
Close:
- XenBusIo->UnregisterWatch (XenBusIo, Dev->StateWatchToken);
+ if (!ResetOnly) {
+ XenBusIo->UnregisterWatch (XenBusIo, Dev->StateWatchToken);
+ }
XenBusIo->XsRemove (XenBusIo, XST_NIL, "ring-ref");
XenBusIo->XsRemove (XenBusIo, XST_NIL, "event-channel");
XenBusIo->XsRemove (XenBusIo, XST_NIL, "protocol");
- XenPvBlockFree (Dev);
+ if (ResetOnly) {
+ XenBusIo->GrantEndAccess (XenBusIo, Dev->RingRef);
+ XenBusIo->EventChannelClose (XenBusIo, Dev->EventChannel);
+ } else {
+ XenPvBlockFree (Dev);
+ }
+}
+
+/**
+ To be called when ExitBootServices has been called.
+
+ This should reset the PV backend without using memory allocation
+ services.
+**/
+VOID
+EFIAPI
+XenPvBlockFrontReset (
+ IN VOID *Context
+ )
+{
+ XEN_BLOCK_FRONT_DEVICE *Dev;
+
+ Dev = Context;
+
+ XenPvBlockFrontShutdown (Dev, TRUE);
}
STATIC
diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.h b/OvmfPkg/XenPvBlkDxe/BlockFront.h
index 7c2d7f2c9e..ebce4fe434 100644
--- a/OvmfPkg/XenPvBlkDxe/BlockFront.h
+++ b/OvmfPkg/XenPvBlkDxe/BlockFront.h
@@ -67,9 +67,19 @@ XenPvBlockFrontInitialization (
OUT XEN_BLOCK_FRONT_DEVICE **DevPtr
);
+/**
+ @param ResetOnly Set to TRUE when called during the ExitBootServices.
+**/
VOID
XenPvBlockFrontShutdown (
- IN XEN_BLOCK_FRONT_DEVICE *Dev
+ IN XEN_BLOCK_FRONT_DEVICE *Dev,
+ IN BOOLEAN ResetOnly
+ );
+
+VOID
+EFIAPI
+XenPvBlockFrontReset (
+ IN VOID *Context
);
VOID
diff --git a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
index bfe7b1a754..3031406980 100644
--- a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
+++ b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
@@ -312,7 +312,7 @@ XenPvBlkDxeDriverBindingStart (
UninitBlockFront:
FreePool (Media);
- XenPvBlockFrontShutdown (Dev);
+ XenPvBlockFrontShutdown (Dev, FALSE);
CloseProtocol:
gBS->CloseProtocol (ControllerHandle, &gXenBusProtocolGuid,
This->DriverBindingHandle, ControllerHandle);
@@ -377,7 +377,7 @@ XenPvBlkDxeDriverBindingStop (
Media = BlockIo->Media;
Dev = XEN_BLOCK_FRONT_FROM_BLOCK_IO (BlockIo);
- XenPvBlockFrontShutdown (Dev);
+ XenPvBlockFrontShutdown (Dev, FALSE);
FreePool (Media);
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 11/11] OvmfPkg/XenBusDxe: Fix XenStoreWaitForEvent use during EBS
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
` (9 preceding siblings ...)
2019-09-13 14:50 ` [PATCH 10/11] OvmfPkg/XenPvBlkDxe: Use XenBusIo->RegisterExitCallback Anthony PERARD
@ 2019-09-13 14:51 ` Anthony PERARD
10 siblings, 0 replies; 25+ messages in thread
From: Anthony PERARD @ 2019-09-13 14:51 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Julien Grall, Jordan Justen, xen-devel,
Anthony Perard, Laszlo Ersek
XenStoreWaitForEvent is going to be called when the ExitBootServices
is signaled, but both CreateEvent and WaitForEvent can't be used.
CreateEvent allocate some memory and WaitForEvent can only be used
when TPL is TPL_APPLICATION.
When ExitBootServices has been called, simply return immediately and
let caller of XenStoreWaitForEvent do a busy loop.
Also cleanup error handling in XenStoreWaitForEvent, WaitForEvent
shouldn't return EFI_UNSUPPORTED anymore.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenBusDxe.c | 2 ++
OvmfPkg/XenBusDxe/XenBusDxe.h | 1 +
OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++----
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
index c71966a666..eb1503ad2b 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.c
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
@@ -264,6 +264,8 @@ NotifyExitBoot (
Dev = Context;
+ Dev->ExitBootNotified = TRUE;
+
//
// First, ask every driver using xenbus to disconnect without
// deallocating memory.
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
index 0e91c24338..80162fc3ff 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.h
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
@@ -79,6 +79,7 @@ struct _XENBUS_DEVICE {
EFI_HANDLE ControllerHandle;
XENIO_PROTOCOL *XenIo;
EFI_EVENT ExitBootEvent;
+ BOOLEAN ExitBootNotified;
EFI_DEVICE_PATH_PROTOCOL *DevicePath;
LIST_ENTRY ChildList;
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 4026c8a829..4f126109d4 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -401,17 +401,22 @@ XenStoreWaitForEvent (
EFI_EVENT TimerEvent;
EFI_EVENT WaitList[2];
+ //
+ // If the ExitBootServices event have been signaled, simply allow to have
+ // busy loop in the caller.
+ //
+ if (xs.Dev->ExitBootNotified) {
+ return EFI_SUCCESS;
+ }
+
gBS->CreateEvent (EVT_TIMER, 0, NULL, NULL, &TimerEvent);
gBS->SetTimer (TimerEvent, TimerRelative, Timeout);
WaitList[0] = xs.EventChannelEvent;
WaitList[1] = TimerEvent;
Status = gBS->WaitForEvent (2, WaitList, &Index);
- ASSERT (Status != EFI_INVALID_PARAMETER);
+ ASSERT_EFI_ERROR (Status);
gBS->CloseEvent (TimerEvent);
- if (Status == EFI_UNSUPPORTED) {
- return EFI_SUCCESS;
- }
if (Index == 1) {
return EFI_TIMEOUT;
} else {
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 25+ messages in thread