Re: [edk2-devel] [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory
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
> ---
> 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
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 UINT32ExpectedRequestId,
> + IN UINT32ExpectedTransactionId,
> + 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 (>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_STATUSStatus;
>
> - if (Message->Header.type == XS_WATCH_EVENT) {
> -CONST CHAR8*WatchEventPath;
> -CONST CHAR8*WatchEventToken;
> -VOID *ConvertedToken;
[edk2-devel] [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory
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
---
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 UINT32ExpectedRequestId,
+ IN UINT32ExpectedTransactionId,
+ 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 (>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_STATUSStatus;
- if (Message->Header.type == XS_WATCH_EVENT) {
-CONST CHAR8*WatchEventPath;
-CONST CHAR8*WatchEventToken;
-VOID *ConvertedToken;
-XENSTORE_WATCH *Watch;
+ while (TRUE) {
-//
-// Parse WATCH_EVENT messages
-// \0\0
-//
-WatchEventPath = Body;
-WatchEventToken = WatchEventPath + AsciiStrSize (WatchEventPath);
+Status = XenStoreReadStore (, sizeof (Header));
+if (Status != XENSTORE_STATUS_SUCCESS) {
+ DEBUG ((DEBUG_ERROR, "XenStore: Error read store (%d)\n", Status));
+ return Status;
+}
-ConvertedToken = (VOID *) AsciiStrHexToUintn (WatchEventToken);
+
