From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 B3ACB2034A893 for ; Fri, 27 Oct 2017 05:35:14 -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 7A9374ACBB; Fri, 27 Oct 2017 12:39:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7A9374ACBB Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-3.rdu2.redhat.com [10.10.122.3]) by smtp.corp.redhat.com (Postfix) with ESMTP id 68B735D753; Fri, 27 Oct 2017 12:38:59 +0000 (UTC) To: "Zeng, Star" , 'Julien Grall' , edk2-devel-01 , "heyi.guo@linaro.org" , "Ni, Ruiyu" Cc: Anthony PERARD , Leif Lindholm , Ard Biesheuvel References: <7516a301-dfbb-5bd4-5e1e-ccd18c26db30@linaro.org> <74e0ac36-195f-9275-922e-e499a513569f@redhat.com> <32b3c432-7f17-c387-5334-a6826d6fa769@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AEB7F@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <68c947a5-34f9-f91e-3670-88182163a2f2@redhat.com> Date: Fri, 27 Oct 2017 14:38:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9AEB7F@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.38]); Fri, 27 Oct 2017 12:39:01 +0000 (UTC) Subject: Re: Xen Console input very slow in recent UEFI 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: Fri, 27 Oct 2017 12:35:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/27/17 05:20, Zeng, Star wrote: > Hi, > > The TimeOut handling in SerialRead() in SerialDxe(MdeModulepkg), IsaSerialRead() in IsaSerialDxe(IntelFrameworkModulePkg) and SerialRead() in PciSioSerialDxe(MdeModulePkg) are consistent, and we did not see this kind of "slow down" before. > > After some investigation, I found it is related to the Timeout value. > > The Timeout is 1000000 (1s) by default to follow UEFI spec. And the Terminal driver will recalculate and set the Timeout value based on the properties of UART in TerminalDriverBindingStart()/TerminalConInTimerHandler(). > > SerialInTimeOut = 0; > if (Mode->BaudRate != 0) { > // > // According to BAUD rate to calculate the timeout value. > // > SerialInTimeOut = (1 + Mode->DataBits + Mode->StopBits) * 2 * 1000000 / (UINTN) Mode->BaudRate; > } > > For example, based on the PCD values of PcdUartDefaultBaudRate, PcdUartDefaultDataBits and PcdUartDefaultStopBits, SerialInTimeOut = (1 + 8 + 1) * 2 * 1000000 / (UINTN) 115200 = 173 (us). > > When SerialDxe is used, > TerminalDriverBindingStart()/TerminalConInTimerHandler() -> > SerialIo->SetAttributes() -> > SerialSetAttributes() -> > SerialPortSetAttributes() > > Some implementations of SerialPortSetAttributes() could handle the input parameters and return RETURN_SUCCESS, for example BaseSerialPortLib16550, then Timeout value will be changed to 173 (us), no "slow down" will be observed. > But some implementations of SerialPortSetAttributes() just return RETURN_UNSUPPORTED, for example XenConsoleSerialPortLib, then Timeout value will be not changed and kept to be 1000000 (1s), "slow down" will be observed. > > Here, how about to? > 1. Handle the input parameters and return status accordingly instead of just returning RETURN_UNSUPPORTED in SerialPortSetAttributes(). > 2. Just return RETURN_SUCCESS instead of RETURN_UNSUPPORTED in SerialPortSetAttributes() if the instance does not care the input parameters at all. I can't speak authoritatively on Xen's behalf, of course, but option (2) appears sane to me -- it is a virtual serial port; in theory it should be able to accept all these parameter values. (My understanding is that the virtual serial port need not change its *behavior* based on the changed attributes. I.e., when keystrokes are available, it doesn't have to slow down itself in providing those keystrokes, just so it match the baud rate.) Thanks Laszlo > > And SerialDxe may can be enhanced like below to be more robust. > > ========== > 6ec9c40f91fc675ee77f3e54aea4e5a41a2de504 > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > index ebcd92726314..060ea56c2b1a 100644 > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > @@ -285,7 +285,21 @@ SerialSetAttributes ( > > Status = SerialPortSetAttributes (&BaudRate, &ReceiveFifoDepth, &Timeout, &Parity, &DataBits, &StopBits); > if (EFI_ERROR (Status)) { > - return Status; > + // > + // If it is just to set Timeout value and unsupported is returned, > + // do not return error. > + // > + if ((Status == EFI_UNSUPPORTED) && > + (This->Mode->Timeout != Timeout) && > + (This->Mode->ReceiveFifoDepth == ReceiveFifoDepth) && > + (This->Mode->BaudRate == BaudRate) && > + (This->Mode->DataBits == (UINT32) DataBits) && > + (This->Mode->Parity == (UINT32) Parity) && > + (This->Mode->StopBits == (UINT32) StopBits)) { > + Status = EFI_SUCCESS; > + } else { > + return Status; > + } > } > > // > ==================== > > > Thanks, > Star > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Julien Grall > Sent: Friday, October 27, 2017 2:32 AM > To: Laszlo Ersek ; edk2-devel-01 ; Zeng, Star ; heyi.guo@linaro.org; Ni, Ruiyu > Cc: Anthony PERARD ; Leif Lindholm ; Ard Biesheuvel > Subject: Re: [edk2] Xen Console input very slow in recent UEFI > > Hi Laszlo, > > Thank you for your help. > > On 26/10/17 16:20, Laszlo Ersek wrote: >> On 10/26/17 17:13, Laszlo Ersek wrote: >>> Hello Julien, >>> >>> On 10/26/17 13:05, Julien Grall wrote: >>>> Hi all, >>>> >>>> I was doing more testing of UEFI in Xen guests and noticed some slow >>>> down when using the shell. The characters are only echoed after a >>>> second or two that is a bit annoying. >>>> >>>> The change that introduced this issue is 4cf3f37c87 "MdeModulePkg >>>> SerialDxe: Process timeout consistently in SerialRead". >>>> >>>> The Serial Driver for Xen PV console is very simple (see >>>> OvmfPkg/Library/XenConsoleSerialPortLib). So I am not sure where the >>>> root cause is. >>>> >>>> Would anyone have any tips on it? >>> >>> The exact same issue has been encountered earlier under QEMU, please >>> refer to the following sub-thread (please read it to end): >>> >>> http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redh >>> at.com >>> >>> The fix was commit 5f0f5e90ae8c ("ArmVirtPkg/FdtPL011SerialPortLib: >>> call PL011UartLib in all SerialPortLib APIs", 2017-08-16). >>> >>> I think if you can implement the same for XenConsoleSerialPortLib, >>> that should return to working state as well. >> >> Hmmm, wait, at a closer look, it looks like >> >> OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> >> already implements that suggestion? (I.e., it sets >> EFI_SERIAL_INPUT_BUFFER_EMPTY in *Control as necessary?) >> >> Are we sure the SerialPortPoll() function works correctly? I don't see >> any MemoryFence() calls in SerialPortPoll(), around checking the >> fields in (*mXenConsoleInterface). Could that be the problem? > > I am not entirely sure. But I added a couple of MemoryFence() in SerialPort just in case to clear that from potential cause: > > XENCONS_RING_IDX Consumer, Producer; > > if (!mXenConsoleInterface) { > return FALSE; > } > > MemoryFence (); > > Consumer = mXenConsoleInterface->in_cons; Producer = mXenConsoleInterface->in_prod; > > MemoryFence (); > > return Consumer != Producer; > > I also added some debug printk (using a different interface) to confirm the value of Consumer and Producer are valid. I can see the Producer increasing every time a key is pressed and then soon followed by SerialPortRead incrementing Consumer. > > I did more debugging and find out the following is happening in TerminalConInTimerHandler (MdeModulePkg/Universal/Console/TerminalDxe) > when a character is received: > > 1) GetControl will return EFI_SERIAL_INPUT_BUFFER_EMPTY unset > => Entering in the loop to fetch character from the serial > 2) GetOneKeyFromSerial() > => Return directly with the character read > 3) Looping as the fifo is not full and no error > 4) GetOneKeyFromSerial() -> SerialRead() > => No more character so SerialPortPoll() will return FALSE and loop until timeout > => Return EFI_TIMEOUT > 5) Exiting the loop from TerminalConInTimerHandler > 6) Characters are printed > > So the step 4) will introduce the timeout seen and delay the echoing of the characters received. > > I could see a couple of solutions to fix it: > 1) Remove the timeout from SerialPortRead and rely on either > a) caller to handle timeout > b) each UART driver > 2) TerminalConInTimerHandler to check at every iteration whether the buffer is empty. > > Any other ideas? > > Cheers, > > -- > Julien Grall > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >