From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-17.smtp-out.eu-west-1.amazonses.com (a7-17.smtp-out.eu-west-1.amazonses.com [54.240.7.17]) by mx.groups.io with SMTP id smtpd.web11.3364.1683581493789985799 for ; Mon, 08 May 2023 14:31:34 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=Tcv5TTw3; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.17, mailfrom: 01020187fd46da1b-71fdeb07-676d-42e5-8eb3-d3cf29dc122c-000000@eu-west-1.amazonses.com) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2; d=ipxe.org; t=1683581492; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References:MIME-Version:Content-Transfer-Encoding; bh=48YiRF62XA2hziecwiXAgAJoZ7HfxCRz4PRgOH0uX7s=; b=Tcv5TTw3e6ZM3YR8iLLxL8PCFU9qGugG4IG/H08q2AF3kDmW+3azN3HmxaHZUumy ABJ6tG3VWAf/b2uFPeIX79C5jsnxb9JXzcbTCNefo5EtVMtA+QqUQBEYBoQ+oPfJ9DG 1i24WWvMuKLHVdFQsqqrI+zyLqFZjTWMTp/VxVfKPcLdEDvMmAV3OhpoumVXbbC7V5R 9ZUKYArZw455RKn6i8z5+mR9HkUaYYGdELvc9J23XG5M/QzHgE2fe87XGBLYJ3uPb9f 3J4Q4cMjX8hLT6Thcg2rcr1kt3dSkiKWD4tJlLs9lBhv1zGnSFTfKyiAtYCo0IYhl8c uVIDK+M5OA== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=shh3fegwg5fppqsuzphvschd53n6ihuv; d=amazonses.com; t=1683581492; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References:MIME-Version:Content-Transfer-Encoding:Feedback-ID; bh=48YiRF62XA2hziecwiXAgAJoZ7HfxCRz4PRgOH0uX7s=; b=tPifmUZq05KaPZQ5YySuM2aFmYtIfmLE/HKILX580WnA5MOFXqk5TJoyQeGF0zfW 9ZOGBYZMZY2WTaXnsrTYOQbCwAyJVBy8CSssJRR9EnnjdtO+6YfWh/uPhsn+j5Fjz3G xBgjXti0Z5DEmsm2jJMz3CApK9gCFw56R4uHMP08= From: "Michael Brown" To: devel@edk2.groups.io Cc: Michael Brown Subject: [PATCH 1/2] OvmfPkg: Clarify invariants for NestedInterruptTplLib Date: Mon, 8 May 2023 21:31:31 +0000 Message-ID: <01020187fd46da1b-71fdeb07-676d-42e5-8eb3-d3cf29dc122c-000000@eu-west-1.amazonses.com> X-Mailer: git-send-email 2.39.0 In-Reply-To: <20230508213100.3949708-1-mcb30@ipxe.org> References: <476bbc17-6484-9afd-9be9-08de14d1d72e@redhat.com> <20230508213100.3949708-1-mcb30@ipxe.org> MIME-Version: 1.0 X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, URIBL_DBL_BLOCKED_OPENDNS,URIBL_ZEN_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Feedback-ID: 1.eu-west-1.fspj4M/5bzJ9NLRzJP0PaxRwxrpZqiDQJ1IF94CF2TA=:AmazonSES X-SES-Outgoing: 2023.05.08-54.240.7.17 Content-Transfer-Encoding: 8bit NestedInterruptTplLib relies on CPU interrupts being disabled to guarantee exclusive (and hence atomic) access to the shared state in IsrState. Nothing in the calling interrupt handler should have re-enabled interrupts before calling NestedInterruptRestoreTPL(), and the loop in NestedInterruptRestoreTPL() itself maintains the invariant that interrupts are disabled at the start of each iteration. Add assertions to clarify this invariant, and expand the comments around the calls to RestoreTPL() and DisableInterrupts() to clarify the expectations around enabling and disabling interrupts. Signed-off-by: Michael Brown --- OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c index e19d98878eb7..e921a09c5599 100644 --- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c +++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c @@ -104,6 +104,7 @@ NestedInterruptRestoreTPL ( // defer our call to RestoreTPL() to the in-progress outer instance // of the same interrupt handler. // + ASSERT (GetInterruptState () == FALSE); if (InterruptedTPL == IsrState->InProgressRestoreTPL) { // // Trigger outer instance of this interrupt handler to perform the @@ -153,6 +154,7 @@ NestedInterruptRestoreTPL ( // // Check shared state loop invariants. // + ASSERT (GetInterruptState () == FALSE); ASSERT (IsrState->InProgressRestoreTPL < InterruptedTPL); ASSERT (IsrState->DeferredRestoreTPL == FALSE); @@ -167,13 +169,17 @@ NestedInterruptRestoreTPL ( // // Call RestoreTPL() to allow event notifications to be - // dispatched. This will implicitly re-enable interrupts. + // dispatched. This will implicitly re-enable interrupts (if + // InterruptedTPL is below TPL_HIGH_LEVEL), even though we are + // still inside the interrupt handler. // gBS->RestoreTPL (InterruptedTPL); // // Re-disable interrupts after the call to RestoreTPL() to ensure - // that we have exclusive access to the shared state. + // that we have exclusive access to the shared state. Interrupts + // will be re-enabled by the IRET or equivalent instruction when + // we subsequently return from the interrupt handler. // DisableInterrupts (); -- 2.39.0