From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 16 Sep 2019 08:41:31 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3A5B73082B6D; Mon, 16 Sep 2019 15:41:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-96.rdu2.redhat.com [10.10.124.96]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6CAA1600C1; Mon, 16 Sep 2019 15:41:29 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Ard Biesheuvel , Julien Grall , Jordan Justen , xen-devel@lists.xenproject.org References: <20190913145100.303433-1-anthony.perard@citrix.com> <20190913145100.303433-7-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <135a493b-bddb-4628-4464-06a4bc1cd029@redhat.com> Date: Mon, 16 Sep 2019 17:41:28 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190913145100.303433-7-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Mon, 16 Sep 2019 15:41:31 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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 > - // \0\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 > + // \0\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) { >