Re: Xen Console input very slow in recent UEFI

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Zeng, Star <star.zeng@intel.com>; heyi.guo@linaro.org; Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>; Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 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
>