From patchwork Fri Sep 13 14:50:55 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anthony PERARD X-Patchwork-Id: 11144869 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id B3F4314E5 for ; Fri, 13 Sep 2019 14:53:16 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 83C2920830 for ; Fri, 13 Sep 2019 14:53:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="cWQ4IXjb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 83C2920830 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1i8mv5-0005Zk-4l; Fri, 13 Sep 2019 14:51:31 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1i8mv4-0005ZV-EF for xen-devel@lists.xenproject.org; Fri, 13 Sep 2019 14:51:30 +0000 X-Inumbo-ID: ea7d0a82-d635-11e9-978d-bc764e2007e4 Received: from esa1.hc3370-68.iphmx.com (unknown [216.71.145.142]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id ea7d0a82-d635-11e9-978d-bc764e2007e4; Fri, 13 Sep 2019 14:51:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1568386267; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=ZzMuAekQ0s9/ThuMWqy291Ul+rre0jXW0+bqi1wWX1E=; b=cWQ4IXjbUU/KB+uNsRiF29db7dCnVirMA9WX1skaT/za5pkaFfDTg7Ne ZsLR+TyqlShaoYzoFhcxduCp0yoZmenkhVQSBmOJiU0A6byeLK6llxr6o hzBD9ScZnypGi1JL5SlMQph8ZMo5aIJtyo+uDh8fuj32O0P7idwHj0Ieu A=; Authentication-Results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=anthony.perard@citrix.com; spf=Pass smtp.mailfrom=anthony.perard@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa1.hc3370-68.iphmx.com: no sender authenticity information available from domain of anthony.perard@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa1.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa1.hc3370-68.iphmx.com: domain of anthony.perard@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa1.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ~all" Received-SPF: None (esa1.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa1.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: 438aV6qfDMfFLKwz7+b256cgHR6jsDeUAT3aYMe98K54x5Kw+DD+JjwzgHc0JvX/ZDk39Y6COc tuIPX1b4FHKirXWaUEVlBDTuKgbOHruuqK6Cl3NaBYCr2+zodcDGmX9FnAeQQ2nwExGXQfyMot CzFPsAZMaVTYa2pR9Sv7OypMAoRznJ/lGIv3gounik3jQz8Tenu/xmIM+hAfoI4ayUDL+we9Tc in9zbAQitbvc/j2aq7/QvZJg0wYQe3E+mvgn+pPQUY3gFzlqv1/SgSwFpnijMNa4Ct6h9f2lA6 vBk= X-SBRS: 2.7 X-MesageID: 5595146 X-Ironport-Server: esa1.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.64,501,1559534400"; d="scan'208";a="5595146" From: Anthony PERARD To: Date: Fri, 13 Sep 2019 15:50:55 +0100 Message-ID: <20190913145100.303433-7-anthony.perard@citrix.com> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190913145100.303433-1-anthony.perard@citrix.com> References: <20190913145100.303433-1-anthony.perard@citrix.com> MIME-Version: 1.0 Subject: [Xen-devel] [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Ard Biesheuvel , Jordan Justen , Julien Grall , Anthony Perard , xen-devel@lists.xenproject.org, Laszlo Ersek Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Acked-by: Laszlo Ersek --- 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 - // \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) {