Re: Xen Console input very slow in recent UEFI

[Julien Grall <julien.grall@linaro.org>]

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