From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::243; helo=mail-wm0-x243.google.com; envelope-from=julien.grall@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F24B72034A886 for ; Fri, 27 Oct 2017 06:15:30 -0700 (PDT) Received: by mail-wm0-x243.google.com with SMTP id r68so3782037wmr.3 for ; Fri, 27 Oct 2017 06:19:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=crQyx8tM1f7rHZk04lRwelcHX/hq7bgefqn33Obl+f8=; b=blXzQbDKSHXmG8DJW+oYdeXh/k0v01ZvvwpPq+bcfvtX5wJoynTLHC7RJaT3ovuIYO c/DLv9wAgxl/IlaelgJ6jY31IuymGCD6n2oJF8kUwyCiyOEds2KiMD2TelWnNh4QTDMs 6ShwJz/Kn+nAnzlhMQK1OYlxFd13aqSAjmoU0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=crQyx8tM1f7rHZk04lRwelcHX/hq7bgefqn33Obl+f8=; b=EbQF81vlsaEWbdWAhXaMB3DK8zcEy0y+I6BQ4NuQ63nH4kc5LatTbiMw4KncVKnUaZ taY8N+yIUb3NUwq+FgXNECxso5OWP9UiBbRGZXaMFkSn9NzImfp5BeOisTcz9Xkq5PpI r95ivs2cUTgal2f26AIF3eBWDrIpsMJcRwQCIXPtI5rInhoEmcWVbIuaa2zBsCMrzxbl +7y7B2PxMP6XbFJi53LKc2TOkE/UMKzIZK9vhOUYTxcQB7JFk5tr5QZsNECkm/GcVW9I U7slTTHCNaSo9gjhEM7x2c0qSwism87YRIgu6l3nTaGW3znUjY2V+MU2satTb2df/TFd v42Q== X-Gm-Message-State: AMCzsaWcyGFhrd4gMLHBul+I///o55BJXV35vU8lQIXlbYCf08pSemlx yi1lStRZBH/F7xdB2UcixSczEA== X-Google-Smtp-Source: ABhQp+TeBANYISh47uze4HCf/9nyTRY5CR/6rKHl8+yhZIrYHZEwdPsdk4SHjy1TMUFGaD7oB6yNlQ== X-Received: by 10.80.152.229 with SMTP id j92mr585411edb.279.1509110353780; Fri, 27 Oct 2017 06:19:13 -0700 (PDT) Received: from ?IPv6:::1? ([2001:41d0:1:6c23::1]) by smtp.gmail.com with ESMTPSA id x10sm5690221edb.24.2017.10.27.06.19.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 27 Oct 2017 06:19:13 -0700 (PDT) To: Laszlo Ersek , "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: Julien Grall Message-ID: Date: Fri, 27 Oct 2017 14:19:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <68c947a5-34f9-f91e-3670-88182163a2f2@redhat.com> 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 13:15:31 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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. >> + (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, -- Julien Grall