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 0488D21E2DA49 for ; Tue, 15 Aug 2017 18:59:54 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D2F0B62647; Wed, 16 Aug 2017 02:02:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D2F0B62647 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-68.phx2.redhat.com [10.3.116.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 793EE18C54; Wed, 16 Aug 2017 02:02:17 +0000 (UTC) To: "Kinney, Michael D" , "Zeng, Star" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , Heyi Guo , Ard Biesheuvel References: <1501835363-61956-1-git-send-email-star.zeng@intel.com> From: Laszlo Ersek Message-ID: <49b59f2c-6e0d-57af-e72e-7de0a7470d16@redhat.com> Date: Wed, 16 Aug 2017 04:02:16 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 16 Aug 2017 02:02:19 +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 01:59:54 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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