From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by mx.groups.io with SMTP id smtpd.web11.11211.1675938488424886394 for ; Thu, 09 Feb 2023 02:28:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ventanamicro.com header.s=google header.b=mmMwJQso; spf=pass (domain: ventanamicro.com, ip: 209.85.216.41, mailfrom: sunilvl@ventanamicro.com) Received: by mail-pj1-f41.google.com with SMTP id pj3so1687642pjb.1 for ; Thu, 09 Feb 2023 02:28:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=7+E+nK7sw0d02D0KlqBY7i1VLlq/kA6M/jboPuZ4VnA=; b=mmMwJQsodQBQUJ4yh7t7v5HKSJnwNT2UWtlDt5WEZJpDo8wvdAQfOZ3lRrYMT8m7sp uVGjlDtPnWdCQLeFbpgE2gJyURZRKM+aEnLMbfCv9YTs+USuH2/T0+VyGUyYacsF8lll BTr3m2Ozt/VoJAVo+OB10FIdKZxuTQPjS7WtHgh30JQc7AQIX2SeZKyHDF9u1xN7I4pd ShWWGXNVHIddGYCJvocWXX1HhX3fPuiJeFF1hX/XaAk24iR+Ze9vPOtYWKAgT7FsQYJD EQ35nfvpXdmUBofzhlM3Z3MywLg0h4qloEVQcL4nzzPY9s86DzHTS4EKWmsQ9ifwFODh XMrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=7+E+nK7sw0d02D0KlqBY7i1VLlq/kA6M/jboPuZ4VnA=; b=TEGjnf35Mjm5WcY6eAiztGp4fbv7Wcy7wSt8SsYDG68sZ9VnVpVGGo4upe1nf4PSWh C7yfDZ5VwImHOuHCtXKigMDIjTD6AWvqj6+y938cit0PoCqBMLyo2A2RxP9UZQ5MZL0V pGzVGsuCyXdW3hVBsw4kvdnLN6KsUxWUE9EA/MkuizPNZNcccYbImZdu3U6sUhLUBh2u vNJwPM/aR0Nak2nutaPV/NJne/hKfxFfMhV5btfQg+T3PMxrEMykm2MxKwbR7p+6dHuU w0416id/HKlYwqe2jTvD17QoLrOfKQ2XNWP8DacxhtkSicx/GwNXbh7nlHyGVMGWYRNM pK3g== X-Gm-Message-State: AO0yUKWWgu06seqYrcUxeA64FhuxQm1L2aqU39IpRzjIEQw78DZlBxcX W7hqWC6FctmpKLmUjlNvYT4Kqw== X-Google-Smtp-Source: AK7set8sx2ed7EuRaDa2hI+xUwG1zRs9ikxyi5opYDSDTzTufji9MDC2WPlUBZ6qYZPS89Skb8+BgQ== X-Received: by 2002:a17:902:e311:b0:198:f6fe:f1ed with SMTP id q17-20020a170902e31100b00198f6fef1edmr9260211plc.42.1675938487732; Thu, 09 Feb 2023 02:28:07 -0800 (PST) Return-Path: Received: from sunil-laptop ([49.206.14.226]) by smtp.gmail.com with ESMTPSA id x4-20020a170902ea8400b0019a5f1410d1sm943087plb.43.2023.02.09.02.28.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Feb 2023 02:28:07 -0800 (PST) Date: Thu, 9 Feb 2023 15:58:00 +0530 From: "Sunil V L" To: Michael Brown Cc: devel@edk2.groups.io, Eric Dong , Ray Ni , Rahul Kumar , Daniel Schaefer , Gerd Hoffmann , Abner Chang Subject: Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 05/20] UefiCpuPkg: Add CpuTimerDxe module Message-ID: References: <20230128191807.2080547-1-sunilvl@ventanamicro.com> <20230128191807.2080547-6-sunilvl@ventanamicro.com> <0102018635ae8953-b7e0dd96-6282-4365-a8da-f3696a61a20c-000000@eu-west-1.amazonses.com> MIME-Version: 1.0 In-Reply-To: <0102018635ae8953-b7e0dd96-6282-4365-a8da-f3696a61a20c-000000@eu-west-1.amazonses.com> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Feb 09, 2023 at 10:17:56AM +0000, Michael Brown wrote: > > +/** > > + 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. > > Hi Michael!, Thanks a lot for this feedback and background. We are aware of few issues in this module. Currently, it is mostly porting what exists today in edk2-platforms repo. We want to add all these additional fixes after this basic thing is merged. That way we will have git history instead of combining all fixes single commit. Andrei has a patch ready and waiting for this to get merged. We can either combine this with his patch or create one more. Would that strategy be fine with you? Thanks! Sunil > 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 >