From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 4465721E1452F for ; Wed, 16 Aug 2017 03:19:22 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A74EE72F0C; Wed, 16 Aug 2017 10:21:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A74EE72F0C Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-59.phx2.redhat.com [10.3.116.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id 260EC7E0B8; Wed, 16 Aug 2017 10:21:45 +0000 (UTC) To: "Zeng, Star" Cc: "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Ni, Ruiyu" , Heyi Guo , Ard Biesheuvel References: <1501835363-61956-1-git-send-email-star.zeng@intel.com> <49b59f2c-6e0d-57af-e72e-7de0a7470d16@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B919406@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <41d9faf8-5792-086e-fdfb-93e7347aa517@redhat.com> Date: Wed, 16 Aug 2017 12:21:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B919406@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 16 Aug 2017 10:21:47 +0000 (UTC) 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: Wed, 16 Aug 2017 10:19:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Star, On 08/16/17 04:22, Zeng, Star wrote: > *Control = 0; > if (!SerialPortPoll ()) { > *Control = EFI_SERIAL_INPUT_BUFFER_EMPTY; > } > return RETURN_SUCCESS; > > As example, the code above (in > OvmfPkg\Library\XenConsoleSerialPortLib\XenConsoleSerialPortLib.c) can > be simply used in SerialPortGetControl(). Thank you for jogging my memory about this. I found the following commit in the git history: ad7f6bc2e116 ("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg", 2015-11-26) and then I found our earlier discussion from 2015: http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com Basically the discussion went like this: (1) In your v1 patch, you disconnected the FdtPL011SerialPortLib instance from the SerialPortExtLib class, and implemented the new functions (GetControl(), SetControl(), SetAttributes()) immediately by calling PL011UartLib. (2) I requested that first we just copy the existing code from the EmbeddedPkg/Library/SerialPortExtLibNull instance -- which FdtPL011SerialPortLib had used up to that point --, and call PL011UartLib in a separate patch (later, if at all). (3) In your v3 patch, you chose an approach that was different from *both* copying SerialPortExtLibNull *and* calling PL011UartLib: the new functions would simply return RETURN_UNSUPPORTED. (4) Analyzing the TerminalDxe driver at that point, we agreed: - that a retval check had to be added after the GetControl() call in TerminalDxe, - more importantly, that with the error check in place, it was *equivalent* for GetControl() to return RETURN_UNSUPPORTED -- seen in your v3 patch --, or to return RETURN_SUCCESS and set Control to zero -- seen in the original code in SerialPortExtLibNull. In both cases, we expected TerminalDxe to work correctly, given that GetOneKeyFromSerial() would exit the loop on the first iteration anyway. In other words, setting EFI_SERIAL_INPUT_BUFFER_EMPTY in Control (and returning success) would only be an unimportant optimization. (5) We agreed that calling PL011UartLib from the new APIs could be a future feature addition. And that's the status what we have right now. Except, with the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe: Process timeout consistently in SerialRead", 2017-07-18), setting EFI_SERIAL_INPUT_BUFFER_EMPTY in Control is no longer an "unimportant optimization", because now GetOneKeyFromSerial() takes *very long* to exit the loop on the first iteration. So even the first iteration must be prevented if the input queue is empty, and that requires implementing GetControl() for real. I think I'll submit an FdtPL011SerialPortLib patch, similar to your v1 approach. PL011UartLib implements these functions; we should use them. Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, August 16, 2017 10:02 AM > To: Kinney, Michael D ; 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 > > On 08/16/17 01:59, Kinney, Michael D wrote: >> 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.inf >> >> 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. > > The issue is different; I checked gBS->Stall() in the above context > and it waits for the appropriate time (usually just 1-2 microseconds > more than requested). > > Instead, the following happens: > > - TerminalConInTimerHandler() in > "MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c" calls > SerialIo->GetControl(). > > - If the GetControl() call fails (for any reason), or the returned > Control word has EFI_SERIAL_INPUT_BUFFER_EMPTY clear, then > TerminalConInTimerHandler() enters a loop: > > // > // Fetch all the keys in the serial buffer, > // and insert the byte stream into RawFIFO. > // > > - The loop body calls GetOneKeyFromSerial() --> SerialIo->Read(), with > a 1 byte buffer. > > - The loop runs until the "raw data FIFO buffer" is filled completely > (256 byte is the size, apparently -- RAW_FIFO_MAX_NUMBER), or > GetOneKeyFromSerial() returns an error. > > - The SerialPortGetControl() function in > "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" > returns constant RETURN_UNSUPPORTED. According to the library class > header "MdePkg/Include/Library/SerialPortLib.h", this is a valid thing > to do ("The serial device does not support this operation"). However, > it will cause TerminalConInTimerHandler() to *always* enter the loop, > even if there is no pending data to read. > > - Because the input queue is empty, GetOneKeyFromSerial() will take a > full second before it times out. > > - And the final piece of the puzzle is that the event associated with > TerminalConInTimerHandler() is signaled at 50Hz (0.02s period, see > KEYBOARD_TIMER_INTERVAL), initially. It can dynamically adjust its > frequency to the serial device's timeout, but in practice, as soon as > the current TerminalConInTimerHandler() frame returns, there's > (apparently) another execution queued already. So basically we're > stuck in the timer event handler. > > I think we should implement the missing ("unsupported") functions in > "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" and > possibly in > "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c". > I believe we could use > "ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c" as an > example. > > Thanks > Laszlo >