From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A36B921E2DA67 for ; Tue, 15 Aug 2017 16:56:44 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Aug 2017 16:59:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,380,1498546800"; d="scan'208";a="124007150" Received: from orsmsx101.amr.corp.intel.com ([10.22.225.128]) by orsmga002.jf.intel.com with ESMTP; 15 Aug 2017 16:59:09 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX101.amr.corp.intel.com (10.22.225.128) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 15 Aug 2017 16:59:09 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.211]) by ORSMSX158.amr.corp.intel.com ([169.254.10.102]) with mapi id 14.03.0319.002; Tue, 15 Aug 2017 16:59:09 -0700 From: "Kinney, Michael D" To: Laszlo Ersek , "Zeng, Star" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Ni, Ruiyu" , Heyi Guo , "Ard Biesheuvel" Thread-Topic: [edk2] [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead Thread-Index: AQHTDPvXGazG6aStqEuD+9AfjVcvz6KGmFcA//+PqFA= Date: Tue, 15 Aug 2017 23:59:08 +0000 Message-ID: References: <1501835363-61956-1-git-send-email-star.zeng@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.22.254.138] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Aug 2017 23:56:44 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, gBS->Stall() layers on top of the Metronome Architectural Protocol. armvirtqemu.dsc: EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf And this implementation layers on top of a TimerLib armvirtqemu.dsc: TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.i= nf There are a couple PCDs involved in this module and lib. Maybe the ArmVirtPkg needs to set some different PCD values to get a more accurate gBS->Stall() when running through QEMU. Best regards, Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > Behalf Of Laszlo Ersek > Sent: Tuesday, August 15, 2017 4:31 PM > To: Zeng, Star ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu ; Heyi Guo > ; Ard Biesheuvel > > Subject: Re: [edk2] [PATCH] MdeModulePkg SerialDxe: Process > timeout consistently in SerialRead >=20 > Hi Star, >=20 > On 08/04/17 10:29, Star Zeng wrote: > > https://lists.01.org/pipermail/edk2-devel/2017- > July/012385.html > > reported the timeout processing in SerialRead is not > consistent. > > > > Since SerialPortPoll only checks the status of serial port and > > returns immediately, and SerialPortRead does not really > implement > > a time out mechanism and will always wait for enough input, > > it will cause below results: > > 1. If there is no serial input at all, this interface will > return > > timeout immediately without any waiting; > > 2. If there is A characters in serial port FIFO, and caller > requires > > A+1 characters, it will wait until a new input is coming and > timeout > > will not really occur. > > > > This patch is to update SerialRead() to check SerialPortPoll() > and > > read data through SerialPortRead() one byte by one byte, and > check > > timeout against mSerialIoMode.Timeout if no input. > > > > Cc: Heyi Guo > > Cc: Ruiyu Ni > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Star Zeng > > --- > > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 18 > ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > index d2383e56dd8f..43d33dba0c2a 100644 > > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > @@ -465,11 +465,25 @@ SerialRead ( > > ) > > { > > UINTN Count; > > + UINTN TimeOut; > > > > Count =3D 0; > > > > - if (SerialPortPoll ()) { > > - Count =3D SerialPortRead (Buffer, *BufferSize); > > + while (Count < *BufferSize) { > > + TimeOut =3D 0; > > + while (TimeOut < mSerialIoMode.Timeout) { > > + if (SerialPortPoll ()) { > > + break; > > + } > > + gBS->Stall (10); > > + TimeOut +=3D 10; > > + } > > + if (TimeOut >=3D mSerialIoMode.Timeout) { > > + break; > > + } > > + SerialPortRead (Buffer, 1); > > + Count++; > > + Buffer =3D (VOID *) ((UINT8 *) Buffer + 1); > > } > > > > if (Count !=3D *BufferSize) { > > >=20 > This patch breaks the ArmVirtQemu platform's boot process -- it > seems to > fall into an infinite loop. I guess the above loop(s) never > complete? >=20 > If I revert this patch on top of current master (af0364f01e8c, > "Nt32/PlatformBootManagerLib: Enable STD_ERROR on all consoles", > 2017-08-14), then ArmVirtQemu boots fine. >=20 > I found this commit with bisection: >=20 > > 4cf3f37c87ba1f9d58072444bd735e40e4779e70 is the first bad > commit > > commit 4cf3f37c87ba1f9d58072444bd735e40e4779e70 > > Author: Star Zeng > > Date: Tue Jul 18 16:32:16 2017 +0800 > > > > MdeModulePkg SerialDxe: Process timeout consistently in > SerialRead > > > > https://lists.01.org/pipermail/edk2-devel/2017- > July/012385.html > > reported the timeout processing in SerialRead is not > consistent. > > > > Since SerialPortPoll only checks the status of serial port > and > > returns immediately, and SerialPortRead does not really > implement > > a time out mechanism and will always wait for enough > input, > > it will cause below results: > > 1. If there is no serial input at all, this interface will > return > > timeout immediately without any waiting; > > 2. If there is A characters in serial port FIFO, and > caller requires > > A+1 characters, it will wait until a new input is coming > and timeout > > will not really occur. > > > > This patch is to update SerialRead() to check > SerialPortPoll() and > > read data through SerialPortRead() one byte by one byte, > and check > > timeout against mSerialIoMode.Timeout if no input. > > > > Cc: Heyi Guo > > Cc: Ruiyu Ni > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Star Zeng > > Reviewed-by: Ruiyu Ni > > > > :040000 040000 aaa8b75d378ec613b828db938c36a16723583906 > d034044918d7f5b8d30783e0a9eccdf3cf21e5c5 M MdeModulePkg >=20 > Bisection log: >=20 > > git bisect start > > # bad: [af0364f01e8cac95afad01437f13beef90f6640b] > Nt32/PlatformBootManagerLib: Enable STD_ERROR on all consoles > > git bisect bad af0364f01e8cac95afad01437f13beef90f6640b > > # good: [c325e41585e374d40fb36b434e61a1ab0fca5b1c] > MdeModulePkg: Fix coding style issues > > git bisect good c325e41585e374d40fb36b434e61a1ab0fca5b1c > > # good: [636cda51903b4b28e1c9b099c4f22a84e61b09da] > OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and > VARS_SPARE_SIZE macros > > git bisect good 636cda51903b4b28e1c9b099c4f22a84e61b09da > > # good: [997b2c543751cb4a3473270c1a7016ade311f01b] > BaseTools/GenCrc32: Fix a bug to hand empty file for decode > > git bisect good 997b2c543751cb4a3473270c1a7016ade311f01b > > # good: [f1658838c267723139711c0b15d98a74980ae4c5] > OvmfPkg/IoMmuDxe: abort harder on memory encryption mask > failures > > git bisect good f1658838c267723139711c0b15d98a74980ae4c5 > > # bad: [9458afa33728e64049d465f052b2c5c3ca3e881c] > IntelFrameworkModulePkg: Fix Xcode 9 Beta treating 32-bit left > shift as undefined > > git bisect bad 9458afa33728e64049d465f052b2c5c3ca3e881c > > # good: [6e414300b5f19d3045a0d21ad90ac2fe965478a5] > EmbeddedPkg/AndroidFastboot: split android boot header > > git bisect good 6e414300b5f19d3045a0d21ad90ac2fe965478a5 > > # bad: [416d48f755518fd1d202b97be2e9944df6e8f0d4] > ShellPkg/drivers: Show Image Name in non-SFO mode > > git bisect bad 416d48f755518fd1d202b97be2e9944df6e8f0d4 > > # bad: [2913ebb2b550f50a14f105e26995dd095e627ba4] > NetworkPkg/HttpBootDxe: Refine the coding style. > > git bisect bad 2913ebb2b550f50a14f105e26995dd095e627ba4 > > # bad: [9e2a8e928995c3b1bb664b73fd59785055c6b5f6] > OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty > S3_CONTEXT > > git bisect bad 9e2a8e928995c3b1bb664b73fd59785055c6b5f6 > > # bad: [4cf3f37c87ba1f9d58072444bd735e40e4779e70] MdeModulePkg > SerialDxe: Process timeout consistently in SerialRead > > git bisect bad 4cf3f37c87ba1f9d58072444bd735e40e4779e70 > > # first bad commit: [4cf3f37c87ba1f9d58072444bd735e40e4779e70] > MdeModulePkg SerialDxe: Process timeout consistently in > SerialRead >=20 > In retrospect, I see that you asked me for feedback in the > original > discussion, at the following URL: >=20 > https://lists.01.org/pipermail/edk2-devel/2017- > July/012406.html >=20 > Unfortunately, this was on July 18th, and I was on vacation > then. (I > think I configured my email account to send an automated out-of- > office > reply.) >=20 > Looking at the patch now, one idea I have is to keep the time > running > across all bytes read; that is, use mSerialIoMode.Timeout as a > global > timeout for the entire SerialRead() call. >=20 > Unfortunately, the UEFI-2.7 spec defines the > "SERIAL_IO_MODE.Timeout" > field, and the Timeout parameter of > EFI_SERIAL_IO_PROTOCOL.SetAttributes(), inconsistently with each > other. > First it says (about the field): >=20 > Timeout If applicable, the number of microseconds to wait > before > timing out a Read or Write operation. >=20 > (This is compatible with my idea.) >=20 > But then it says (in the general description, and about the > SetAttributes() parameter): >=20 > The default attributes for all UART-style serial device > interfaces > are: [...] a 1,000,000 microsecond timeout *per character* > [...] >=20 > Timeout The requested time out *for a single character* in > microseconds. This timeout applies to both the > transmit and > receive side of the interface. A Timeout value of 0 > will use > the device's default time out value. >=20 > (Emphases mine.) >=20 > ... I added some debug messages to the loops, and the first > invocation > of the function seems to hang with the following parameters: >=20 > > SerialRead: BufferSize=3D1 mSerialIoMode.Timeout=3D1000000 >=20 > That is, the caller wants to read a single character (which is > not > arriving). The timeout is the default 1 second. However, the > wait takes > much-much longer than 1 second (it appears to be infinite). It > seems > that gBS->Stall(10) takes much longer than 10 microseconds. > According to > the UEFI spec, >=20 > The Stall() function stalls execution on the processor for at > least > the requested number of microseconds. [...] >=20 > So Stall() is allowed to wait longer (possibly a lot longer) -- > I guess > it depends on some timer's resolution. >=20 > I have another idea related to this -- let me research it a bit > later. I > might post some patches. >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel