From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.12500.1683331042384684656 for ; Fri, 05 May 2023 16:57:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=sLb9Cw1Z; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id BEBEA63235 for ; Fri, 5 May 2023 23:57:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3238AC433EF for ; Fri, 5 May 2023 23:57:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1683331041; bh=LRPFyJxzpUhUEAaOoC6j+X1YjelW5K1BUMypf8T6y6c=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=sLb9Cw1Z4yfTi54qkXRxt2QgtJdCld9rJQRA6Ea4d/ET1HnF9b1lNQSwV+Ef7WU0d /r0DZsrjLKIU3Z2gQo6MzYSrS8auS3e6mBERp744TSTerM0i4FsUaW9OnvRH/zMn2Z Wd5sXz0qzyOEt7qak//znhqiu8dgricF/UQETTglQwx2usbfUEaa8dtilmDV7Unpld eikbozXqNQ8ZB7v9fyipc9kEusHDzylGmpkmda8kzLwTuVEWcUo2JQ+93gaG9EmcgO bCXA0RXKwmg0i6pK1Zc68QIExZuFazq3nVDbPl0yAbjzr25hEgrxDwy3bMENz04fWc f/35M/2AhrKqA== Received: by mail-lf1-f46.google.com with SMTP id 2adb3069b0e04-4f0108a7d20so2688778e87.3 for ; Fri, 05 May 2023 16:57:21 -0700 (PDT) X-Gm-Message-State: AC+VfDy9EXRg8IyN4VvoKCgw5nuFhblpryhB1v/WbxiYnX5ucIMBqlRr 3i4MYsml55XG/1yUD9kHH++0gpbIuUMuiCQPMWE= X-Google-Smtp-Source: ACHHUZ5fALSAFWVQuhqDCMjwDkmIHLEdX979tISl3AoP5CzB/K769jfKTlUNn6D7Q/7CLSApe5sjJD8KhCxUC5s5SIQ= X-Received: by 2002:ac2:4184:0:b0:4eb:4002:a5ca with SMTP id z4-20020ac24184000000b004eb4002a5camr809920lfh.66.1683331039155; Fri, 05 May 2023 16:57:19 -0700 (PDT) MIME-Version: 1.0 References: <20230503071954.266637-1-kraxel@redhat.com> <01020187ec402266-6d4dee99-5a0d-4105-abaf-419c2a5607cc-000000@eu-west-1.amazonses.com> <01020187ee3d92cc-eb212c44-2e49-4ca2-992c-a2d7d3b03f6f-000000@eu-west-1.amazonses.com> In-Reply-To: <01020187ee3d92cc-eb212c44-2e49-4ca2-992c-a2d7d3b03f6f-000000@eu-west-1.amazonses.com> From: "Ard Biesheuvel" Date: Sat, 6 May 2023 01:57:07 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged. To: Michael Brown Cc: devel@edk2.groups.io, lersek@redhat.com, kraxel@redhat.com, Oliver Steffen , Pawel Polawski , Jiewen Yao , Ard Biesheuvel , Jordan Justen Content-Type: text/plain; charset="UTF-8" On Sat, 6 May 2023 at 01:27, Michael Brown wrote: > > On 05/05/2023 19:56, Laszlo Ersek wrote: > > I don't like the patch. For two reasons: > > > > (1) It papers over the actual issue. The problem should be fixed where > > it is, if possible. > > Agreed, but (as you have shown in > https://bugzilla.redhat.com/show_bug.cgi?id=2189136) the bug lies in > Windows code rather than in EDK2 code. If the goal is to allow these > buggy Windows builds to still be used with OVMF, then the only option is > to paper over the issue. We should do this only if it can be proven > safe to do so, of course. > > > (2) With the patch applied, NestedInterruptRaiseTPL() can return > > TPL_HIGH_LEVEL (as "InterruptedTPL"). Consequently, > > TimerInterruptHandler() [OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c] > > may pass TPL_HIGH_LEVEL back to NestedInterruptRestoreTPL(), as > > "InterruptedTPL". > > > > I believe that this in turn may invalidate at least one comment in > > NestedInterruptRestoreTPL(): > > > > // > > // Call RestoreTPL() to allow event notifications to be > > // dispatched. This will implicitly re-enable interrupts. > > // > > gBS->RestoreTPL (InterruptedTPL); > > > > Restoring TPL_HIGH_LEVEL does not re-enable interrupts -- nominally anyways. > > I agree that the comment is invalidated, but as far as I can tell the > logic remains safe. > > I will put together a patch to update the comments in > NestedInterruptTplLib to address the possibility of an interrupt > occurring (illegally) at TPL_HIGH_LEVEL. > > > (a) Make LocalApicTimerDxe Xen-specific again. It's only the OVMF Xen > > platform that really *needs* NestedInterruptTplLib. (Don't get me wrong: > > NestedInterruptTplLib is technically correct in all circumstances, but > > in practice it happens to be too strict.) > > > > (b) For the non-Xen OVMF platforms, re-create a LocalApicTimerDxe > > variant that effectively has commits a086f4a63bc0 and a24fbd606125 > > reverted. (We should keep 9bf473da4c1d.) This returns us to > > pre-239b50a86370 status -- that is, a timer interrupt handler that (a) > > does not try to be smart about nested interrupts, therefore one that is > > much simpler, and (b) is more tolerant of the Windows / cdboot.efi spec > > violation, (c) is vulnerable to the timer interrupt storm seen on Xen, > > but will never run on Xen. (Only the OVMF Xen platform is supposed to be > > launched on Xen.) > > I'm less keen on this because it reduces the runtime exposure of a very > complex piece of code, and will effectively cause that code to become > unmaintained. > > It's also satisfying (to me) that NestedInterruptTplLib provides a > provable upper bound on stack consumption due to interrupts, which can't > be guaranteed by the simpler pre-239b50a86370 scheme. > > Could we defer judgement until after I've fully reasoned through (and > documented) how NestedInterruptTplLib will work in the presence of > interrupts occurring at TPL_HIGH_LEVEL? > Would it be feasible for our firmware implementation to disable the timer interrupt at the timer end as well? E.g., RaiseTPL(HIGH):: CLI disarm timer RestoreTPL:: re-arm timer STI