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:[email protected]] On Behalf Of Julien
> Grall
> Sent: Friday, October 27, 2017 2:32 AM
> To: Laszlo Ersek <[email protected]>; edk2-devel-01
> <[email protected]>; Zeng, Star <[email protected]>;
> [email protected]; Ni, Ruiyu <[email protected]>
> Cc: Anthony PERARD <[email protected]>; Leif Lindholm
> <[email protected]>; Ard Biesheuvel <[email protected]>
> 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
> [email protected]
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
