From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [40.107.220.77]) by mx.groups.io with SMTP id smtpd.web12.15010.1634050620123986066 for ; Tue, 12 Oct 2021 07:57:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nvidia.com header.s=selector2 header.b=HW1IJQ63; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: nvidia.com, ip: 40.107.220.77, mailfrom: ashishsingha@nvidia.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kkwUtbAkeRrqzYUuH+2fFRnlBG/EQDAM/k+zjg+i+pL/AXgyE7EBYbahxCoFJnD20H0zbvd4ODEEsWkyn2JJLXPk/nnPr4hRVVnPaztjWKtOPJmMl+z283Ellx/UUOonA9YrE/ztIHg/Y6W+UHHtX9NP0tKgt0awDLV2HyeXIpV5C8ItRiJAifsPHlwcGUASg+2yJd4h7rnFAs8Jo3uyMwbTpkCkPWxNrzYkXJtQPMWw7GbFGs/Hvxzg7jCAYDkN/BgAH2XjZ5AUItnOkmAJVa9EBIyBp8hRvoGRqQIOc1RkPAktAJN7r57QB4yoxLkbxZHFA60yEUg6pkvFPJ+5+g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=TO85TWG+oWxvBDwAnYswAD179Zb+nGhuEEtg4Xzn5Jk=; b=AwnUGWFRiwFbQXfyp1hMDVRu+2bLaCU/Rh5VvytPuqfDqvvJW599e43IiWbA67Ocx5IDG9LQo4QdZBiO5hQ49FQjI9f3zjkd1Yt0ykWBafuYtshkdvCLS5nTEBEb6Sg6kvKZtncAS/ukMLFEXXdphLsO2MYt1Z83XegXyNgd2/cL49bjJMe4UYDd2hGVjjnaElGiHKz2CjdUT7Lx/ccZxdy5MIxIotwAnfdEE37g3/EgDCy5QwT3mkaL3UDUFJXRMACwC3yr5hLYLBdNkETare9plep/JwdA1YsqEby5hVaJ6PA8PJkuK3X9M4o5oxC99Fef8BrixKVsC6N7fJ5Gqw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=TO85TWG+oWxvBDwAnYswAD179Zb+nGhuEEtg4Xzn5Jk=; b=HW1IJQ630ZX/Fm3gyvpHtu+Fb5453Q0hyJsov9MgcXJ66CGJYs1+v+wtVA94QHAVarSCiP2C3qZxpVKXhhxcRviJOrsd2oPWl1jdGt38z5rILVlvMb3QYmTt65d+owk/jOQJRe+K82FDcuGeTHjLbCDy2zoRrbTN92pUIMhoUkgdh6EP66E4s11p/Oyk1r4A9NhyoxEyxbtOS7fOwoUvGNkN/WGbFj5GyzB5NICPpKMIaVgTvrzRWM8wGNrbVs/cjMP6zGCZwB6h7HOkd0Imw9dHFLPOwa/HrtKc1oAo/8E4iXVpnds6EQqoQMJ3A9Rowoc6Eh2M9Mecjvesc1Qeog== Received: from CO6PR12MB5396.namprd12.prod.outlook.com (2603:10b6:303:139::8) by CO6PR12MB5428.namprd12.prod.outlook.com (2603:10b6:5:35c::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4587.18; Tue, 12 Oct 2021 14:56:59 +0000 Received: from CO6PR12MB5396.namprd12.prod.outlook.com ([fe80::3181:5873:ca0f:cf8b]) by CO6PR12MB5396.namprd12.prod.outlook.com ([fe80::3181:5873:ca0f:cf8b%7]) with mapi id 15.20.4587.026; Tue, 12 Oct 2021 14:56:58 +0000 From: "Ashish Singhal" To: Marc Zyngier , Ard Biesheuvel CC: edk2-devel-groups-io , Leif Lindholm , Ard Biesheuvel Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal Thread-Topic: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal Thread-Index: AQHXvuitkDcYT9ItPUy1xjbmHBblWKvOX7wAgACwtICAAGPiiQ== Date: Tue, 12 Oct 2021 14:56:58 +0000 Message-ID: References: <87h7dmpqn2.wl-maz@kernel.org> In-Reply-To: <87h7dmpqn2.wl-maz@kernel.org> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: 3492d706-38ae-1360-4409-451aa75e5c35 authentication-results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: d00aebc1-b8f1-43e2-dd9b-08d98d908a9b x-ms-traffictypediagnostic: CO6PR12MB5428: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: MehDgCMv/092PhWvdXJX6fmtrpFXSg/2pKZUetrw0QRMyoulXi8Czlm22xHhHd2IG3ro+nPVop+dRQuH7QrgbEwdtTM5XBwhyc7/347C0QQZBNwnL/uBML9VEs4bK9qMMs7WPK4B2soYK7CgLfJcJynoWxEfSjrMgn8jMysekeY+kUcXPwlBZ6lMM9WDAlkwBw2AEcBiMhvpFKfudC+Jv4w1rU71CLFLbv9SaWlPmeZhqDDX2HgD92vulQt1GYyEjyvE22i6o/eENN8Ti3ouP84Hw/1Ty/yphi4ZRZJBMyytS6kSk1wOgI1abASKEAtEFVVdCci7m3hrMUO4aZnc8OdCDMx98Q0kLuCdL4AU1cM6o0zLG9Xy8tdRLKz/81mlGliIZOKgyH8Gl9QTseHAdKSyidgDLqgI5EWO6IMyKSNQ8belQh7jBtZF3jwCbJLS8HrF1PpX2dWS+lCPXV+p1JSHXK6/2UII7XP6tq9mJVJ5ZOSZ+P92idYrq6MszUWf3mzzzqbiwKQkiBsksT0A828KPbfGijklt8B7LRaJykQV17NcCd+x9Wbn6jFlTC0RF1uTc0ML5SZLLrmS6mg+gHv4zBxz8qdv6/K49LuTFMahUZAKANTkFp6cnWC1GIqJgqXUCm0VSDWeL0qKVTEfa+cxs6MhgeuIMVBYawQnKsOUi9teB9nhzIwxKuho9o7aix1TJhr4M+Tsog0SyA0LHg== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CO6PR12MB5396.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(38070700005)(86362001)(66446008)(5660300002)(6506007)(53546011)(66556008)(66476007)(64756008)(19627405001)(8676002)(7696005)(4326008)(110136005)(2906002)(54906003)(76116006)(66946007)(83380400001)(33656002)(8936002)(91956017)(508600001)(52536014)(316002)(9686003)(26005)(122000001)(186003)(38100700002)(55016002)(71200400001);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?tiNwFhQh3MU7XX9e0qCjh0JGPCrTP3qL6iuxK2JhbMABBtozyESiFTE+JLd3?= =?us-ascii?Q?U64OmGogfhe+hT+SgSQBKwcgmlrQkMMoAdnKHuG+zGxq6MLRpt1p5BbsizmZ?= =?us-ascii?Q?rdbhed7wUeA64ymhMfKU7sWO7Kf6SA9hlPGPq63sBzzyOvgA/v+n1IVnDjdE?= =?us-ascii?Q?ahLHW5jU+ZqZ6U7ycwZO1KUeHstrsePYZdFNAwMi/NHEykeHIVla1LOtBiFE?= =?us-ascii?Q?1NPRMt9vJWKrhN6/j8sztxonhxOL0EXXmckME+D9BIwvRlS8RKbIpAQasNRq?= =?us-ascii?Q?iy4OrAP5k3oCu/IO46iiekGeaaHcBBreP+MrI4mNoLXv0QP+01TMfW/1zJh6?= =?us-ascii?Q?eecw7eSNa3wfm3IFeCOxW9ItwFbyN7nU5dtHgGHd6WmGfV054z234EVJW6lf?= =?us-ascii?Q?l5teHMF2/kblIkdqkejvNQ1KJAUgTWAUmrTHFPIVPDgm+dsOQ3qOj5Za1RdF?= =?us-ascii?Q?nXwgbx0ASRNTKB09L+2ljaypKF3k1QCZMvAWdr4IVNs2troEnIPgWbnLIqhG?= =?us-ascii?Q?SvlsRi4scWDW4LTzlgRyM3bJFDxRJHcLPZ0zZlG7m/Ch326tc9xvmom2TLK+?= =?us-ascii?Q?ULjTDdM9kIxN0ccqrdMRsDxxCuEni9gCeX+kFN+MtAUK2+N1jMYrZ8y+grof?= =?us-ascii?Q?6PnDL/EbQbvnKb1KEbu4wi/ogGj6dxKKgy3Ns7StY+J/zkX8kNR/8ws7GxOw?= =?us-ascii?Q?CcOEFy3RCsSM2dsn6/x/9b7+FEipsCfdTKC0/X03zYBzBHjRlmSeq4B3z5Hc?= =?us-ascii?Q?nW8daNYWCH+k6HNkWqKU8hDYvRgnMCxU0xt2HZVmw8PRBWwyqIBB3xETk+5Z?= =?us-ascii?Q?ZmBflWOO24i3pBRVRhnMUwQCZi0usRDkLCGZjRq9jrW/ohoqk6EErTdqW/GL?= =?us-ascii?Q?RTJapI5EBrp8PT0l5FoOmdjS4UBG4VTNM+uoEhToj0d/KUtORNKt5Rn73Jl4?= =?us-ascii?Q?EOnKmeXrrTAUtweyaCEqcYXsOUo/j7zU/k3YuWrPSLZIXZSQXvKE7MticE9a?= =?us-ascii?Q?ZZdBK+ub9lqU54++imL0M/gHBVB8MchZIfUv43YelRqLz6Bo69a9qcNx9AJR?= =?us-ascii?Q?X49g/U/X0Z/4K5XkZ4TeK/7lecQD0kPxxbjcQdBEkXvy86YZnQB6LJzUvXNW?= =?us-ascii?Q?B7ureNKhb2QTDKldsSrWnIKIVjaQ25d5pqY26DFGBYd362BOtwpsAqi0wYNA?= =?us-ascii?Q?e7pjzX3mXWtIuc9mHjmKlr8nZE2Fk2QYc53sJVUSQJWHgpzZ6DNBcB+oasbH?= =?us-ascii?Q?7aMd22DefeRGasb5o0kyqv5D8UK2h9SjsAcunTc7awKI2xXw7RGCQmRXTXf3?= =?us-ascii?Q?RcI=3D?= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5396.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: d00aebc1-b8f1-43e2-dd9b-08d98d908a9b X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Oct 2021 14:56:58.8360 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 71d1POgWSsxBtmrkXS9XxJMxwnFwQqY+orRkQTb4JAsVa24heJxU3VpWI6XcF/lgWTrYooDuGzozFqHPFPDVRA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR12MB5428 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CO6PR12MB5396CA3B6FF8C1807FE881BDBAB69CO6PR12MB5396namp_" --_000_CO6PR12MB5396CA3B6FF8C1807FE881BDBAB69CO6PR12MB5396namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Marc, In the document ARM062-1010708621-30 (AArch64 Programmer's Guides Generic T= imer), towards the end of section 3.4 it says: "When writing an interrupt h= andler for the timers, it is important that software clears the interrupt b= efore deactivating the interrupt in the GIC. Otherwise, the GIC will re-sig= nal the same interrupt again." My change was in accordance with this. We only clear the interrupt when we = update the compare value but were signaling EOI before that going against t= he guidance in the document. Thanks Ashish ________________________________ From: Marc Zyngier Sent: Tuesday, October 12, 2021 2:57 AM To: Ard Biesheuvel ; Ashish Singhal Cc: edk2-devel-groups-io ; Leif Lindholm ; Ard Biesheuvel Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal External email: Use caution opening links or attachments On Mon, 11 Oct 2021 23:24:38 +0100, Ard Biesheuvel wrote: > > (+ Marc) > > On Mon, 11 Oct 2021 at 23:40, Ashish Singhal wr= ote: > > > > In an interrupt handler for the timers, it is important that > > software clears the interrupt before deactivating the interrupt > > in the GIC. Otherwise the GIC will re-signal the same interrupt > > again. > > > > Signed-off-by: Ashish Singhal > > Please don't spam us with almost identical versions of the same patch > without even documenting what the difference is. > > > > --- > > ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerD= xe/TimerDxe.c > > index 0370620fae..46e5bbf287 100644 > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > @@ -300,10 +300,6 @@ TimerInterruptHandler ( > > // > > OriginalTPL =3D gBS->RaiseTPL (TPL_HIGH_LEVEL); > > > > - // Signal end of interrupt early to help avoid losing subsequent tic= ks > > - // from long duration handlers > > - gInterrupt->EndOfInterrupt (gInterrupt, Source); > > - > > // Check if the timer interrupt is active > > if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) { > > > > @@ -335,6 +331,11 @@ TimerInterruptHandler ( > > ArmInstructionSynchronizationBarrier (); > > } > > > > + // In an interrupt handler for the timers, it is important that soft= ware clears the interrupt > > + // before deactivating the interrupt in the GIC. Otherwise the GIC w= ill re-signal the same > > + // interrupt again. > > + gInterrupt->EndOfInterrupt (gInterrupt, Source); > > + > > gBS->RestoreTPL (OriginalTPL); > > } > > This isn't a requirement if you haven't re-enabled interrupts in PSTATE (and the TPL being raised seems to indicate that interrupts are actively masked here). The timer is a level interrupt, and lowering the level takes time. If your GIC implementation is good, it will notice that the lowering of the level quickly, before you can reach the point where you re-enable interrupts. If it is slow (or badly emulated if this is actually done in a hypervisor), you'll get a spurious interrupt. In any case, moving the EOI around doesn't make things any better. You are just moving the goal post, without additional guarantees that the level has been retired. As a consequence, I don't think this patch makes much sense. M. -- Without deviation from the norm, progress is not possible. --_000_CO6PR12MB5396CA3B6FF8C1807FE881BDBAB69CO6PR12MB5396namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Marc,

In the document ARM062-1010708621-30 (AArch64 Programmer's Guides Gene= ric Timer), towards the end of section 3.4 it says: "When writing an i= nterrupt handler for the timers, it is important that software clears the i= nterrupt before deactivating the interrupt in the GIC. Otherwise, the GIC will re-signal the same interrupt again.&qu= ot;

My change was in accordance with this. We only clear the interrupt when we = update the compare value but were signaling EOI before that going against t= he guidance in the document.

Thanks
Ashish
From: Marc Zyngier <maz@= kernel.org>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb@kernel.org>; Ashish Singhal <ashis= hsingha@nvidia.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm= <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>= ;
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Sign= al
 
External email: Use caution opening links or attac= hments


On Mon, 11 Oct 2021 23:24:38 +0100,
Ard Biesheuvel <ardb@kernel.org> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@nvidia.c= om> wrote:
> >
> > In an interrupt handler for the timers, it is important that
> > software clears the interrupt before deactivating the interrupt > > in the GIC. Otherwise the GIC will re-signal the same interrupt > > again.
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
>
> Please don't spam us with almost identical versions of the same patch<= br> > without even documenting what the difference is.
>
>
> > ---
> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/= TimerDxe/TimerDxe.c
> > index 0370620fae..46e5bbf287 100644
> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > @@ -300,10 +300,6 @@ TimerInterruptHandler (
> >    //
> >    OriginalTPL =3D gBS->RaiseTPL (TPL_HIGH_LEVE= L);
> >
> > -  // Signal end of interrupt early to help avoid losing sub= sequent ticks
> > -  // from long duration handlers
> > -  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > -
> >    // Check if the timer interrupt is active
> >    if ((ArmGenericTimerGetTimerCtrlReg () ) & = ARM_ARCH_TIMER_ISTATUS) {
> >
> > @@ -335,6 +331,11 @@ TimerInterruptHandler (
> >      ArmInstructionSynchronizationBarrie= r ();
> >    }
> >
> > +  // In an interrupt handler for the timers, it is importan= t that software clears the interrupt
> > +  // before deactivating the interrupt in the GIC. Otherwis= e the GIC will re-signal the same
> > +  // interrupt again.
> > +  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > +
> >    gBS->RestoreTPL (OriginalTPL);
> >  }
> >

This isn't a requirement if you haven't re-enabled interrupts in
PSTATE (and the TPL being raised seems to indicate that interrupts are
actively masked here).

The timer is a level interrupt, and lowering the level takes time. If
your GIC implementation is good, it will notice that the lowering of
the level quickly, before you can reach the point where you re-enable
interrupts. If it is slow (or badly emulated if this is actually done
in a hypervisor), you'll get a spurious interrupt.

In any case, moving the EOI around doesn't make things any better. You
are just moving the goal post, without additional guarantees that the
level has been retired.

As a consequence, I don't think this patch makes much sense.

        M.

--
Without deviation from the norm, progress is not possible.
--_000_CO6PR12MB5396CA3B6FF8C1807FE881BDBAB69CO6PR12MB5396namp_--