From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-20.smtp-out.eu-west-1.amazonses.com (a7-20.smtp-out.eu-west-1.amazonses.com [54.240.7.20]) by mx.groups.io with SMTP id smtpd.web10.11099.1675937878333485858 for ; Thu, 09 Feb 2023 02:17:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=KnsVyxzV; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.20, mailfrom: 0102018635ae8953-b7e0dd96-6282-4365-a8da-f3696a61a20c-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=1675937876; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=VwNxW0wdgWQET1x6rjWPuqK3Xax7TBBgWmVQQ242Zkk=; b=KnsVyxzVCw9m4sDLU3OHAphUplErAWW6GCcLoH1InUbE0HB/yPqY5Z2Idl3kqevS 1nH1bTjaHItffEbk75gJqckRN2vMGQWxvIdwLmCYw6FwGvUMk+UGREHMEsCPqFa+1gW SQT/KkQUQUbusOAuu0IafntvYd2OSX9bsvdXzV2A8HTPk6NGFsETeZyEX6xpwmAK01a s6PoA6fwPdwXPIjbz/kjaUfcgsPcJ5+0aaHq1zfTCkQ4a762mrNwD2WDbPli436O4Vt C5DjZOW6YUbUF8EYD299DwIVuyQlUhqtfw+7FuAEY1UoHX+7J+e51NRb0xtUTRDYKjx OWrNn3Kxfw== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=ihchhvubuqgjsxyuhssfvqohv7z3u4hn; d=amazonses.com; t=1675937876; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=VwNxW0wdgWQET1x6rjWPuqK3Xax7TBBgWmVQQ242Zkk=; b=XAmGeuzBgoNEqbKGpIw5uL500WGTtnw9ykgnshpnJIx29EO2N5R9sARZEquUKTGa 3V44qhIIzhcrHXrlgtkZvzHzdfSDBnGnzyxZ1zBZhDn7spJpNMVVMbRvX0OH14WJ3GP IqtyGSe2oqYh9eFFaXi0pEDydlxVO3yhkKSZh4gc= Message-ID: <0102018635ae8953-b7e0dd96-6282-4365-a8da-f3696a61a20c-000000@eu-west-1.amazonses.com> Date: Thu, 9 Feb 2023 10:17:56 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 05/20] UefiCpuPkg: Add CpuTimerDxe module To: devel@edk2.groups.io, sunilvl@ventanamicro.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Daniel Schaefer , Gerd Hoffmann , Abner Chang References: <20230128191807.2080547-1-sunilvl@ventanamicro.com> <20230128191807.2080547-6-sunilvl@ventanamicro.com> From: "Michael Brown" In-Reply-To: <20230128191807.2080547-6-sunilvl@ventanamicro.com> X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, URIBL_BLOCKED,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.02.09-54.240.7.20 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit > +/** > + Timer Interrupt Handler. > + > + @param InterruptType The type of interrupt that occured > + @param SystemContext A pointer to the system context when the interrupt occured > +**/ > +VOID > +EFIAPI > +TimerInterruptHandler ( > + IN EFI_EXCEPTION_TYPE InterruptType, > + IN EFI_SYSTEM_CONTEXT SystemContext > + ) > +{ > + EFI_TPL OriginalTPL; > + UINT64 RiscvTimer; > + > + OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); > + if (mTimerNotifyFunction != NULL) { > + mTimerNotifyFunction (mTimerPeriod); > + } > + > + RiscVDisableTimerInterrupt (); // Disable SMode timer int > + RiscVClearPendingTimerInterrupt (); > + if (mTimerPeriod == 0) { > + gBS->RestoreTPL (OriginalTPL); > + RiscVDisableTimerInterrupt (); // Disable SMode timer int > + return; > + } > + > + RiscvTimer = RiscVReadTimer (); > + SbiSetTimer (RiscvTimer += mTimerPeriod); > + gBS->RestoreTPL (OriginalTPL); > + RiscVEnableTimerInterrupt (); // enable SMode timer int > +} This design looks as though it does not support nested timer interrupts. The call to RestoreTPL() may invoke callbacks that may themselves include delay loops that wait upon further timer interrupts. With the above code, those timer interrupts will never arrive since the timer interrupt is disabled at the point that you call RestoreTPL(). This will break device drivers such as those for USB network devices that rely on nested timer interrupts. The easiest fix is to use NestedTimerInterruptLib and change the code as follows: Add at the start of the function: STATIC NESTED_INTERRUPT_STATE NestedInterruptState; Change: OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); to OriginalTPL = NestedInterruptRaiseTPL (); Change: gBS->RestoreTPL (OriginalTPL); RiscVEnableTimerInterrupt (); // enable SMode timer int to RiscVEnableTimerInterrupt (); // enable SMode timer int NestedInterruptRestoreTPL (OriginalTpl, SystemContext, &NestedInterruptState); Note that the timer interrupt is then correctly re-enabled before the call to NestedInterruptRestoreTPL(). NestedInterruptTplLib takes care of the mess required behind the scenes to make this all provably safe. You'll also need to extend Library/NestedInterruptTplLib/Iret.c to support MDE_CPU_RISCV64: it should be fairly obvious what's needed there. See commit https://github.com/tianocore/edk2/commit/a086f4a63b for the equivalent fix being applied to OVMF. You can find the background leading up to the creation of NestedInterruptTplLib at: https://bugzilla.tianocore.org/show_bug.cgi?id=4162 https://github.com/tianocore/edk2/commit/9bf473da4c https://github.com/tianocore/edk2/commit/a24fbd6061 https://github.com/tianocore/edk2/commit/a086f4a63b Thanks, Michael