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 D5CFC2034A881 for ; Fri, 27 Oct 2017 08:40:08 -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 150315D685; Fri, 27 Oct 2017 15:43:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 150315D685 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-122-3.rdu2.redhat.com [10.10.122.3]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0A2FE60618; Fri, 27 Oct 2017 15:43:52 +0000 (UTC) To: Julien Grall , "Zeng, Star" , 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> <68c947a5-34f9-f91e-3670-88182163a2f2@redhat.com> From: Laszlo Ersek Message-ID: <5a867af3-0a19-b7b2-f656-55dde0df1a75@redhat.com> Date: Fri, 27 Oct 2017 17:43:51 +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: 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]); Fri, 27 Oct 2017 15:43:55 +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 15:40:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 10/27/17 15:19, Julien Grall wrote: > Hi Laszlo & Star, > > On 27/10/17 13:38, Laszlo Ersek wrote: >> 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. > > The slow down could be observed on BaseSerialPortLib16550 if you pass > invalid parameters. Therefore EFI_INVALID_PARAMETER will be returned. > >>> 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.) > > That's correct, none of the parameters but Timeout matters for Xen. But > given that other driver are using that value, would not it be better to > use the patch suggested by Start (see below)? > >>> >>> And SerialDxe may can be enhanced like below to be more robust. > > You definitely want such patch in the tree as EFI_UNSUPPORTED is a valid > return parameter and used by other serial driver (for instance > DxeEmuSerialPortLib). > >>> >>> ========== >>> 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) && > > I would also check EFI_INVALID_PARAMETER as the implementation may not > support some values, but it is still fine to modify the Timeout. I think you are right. Thanks Laszlo > >>> +        (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; >>> +    } >>>     } >>>       // >>> ==================== >> > Cheers, >