Re: Xen Console input very slow in recent UEFI

["Zeng, Star" <star.zeng@intel.com>]

I do not think SerialDxe should handle EFI_INVALID_PARAMETER return value from SerialPortSetAttributes().
If the EFI_INVALID_PARAMETER is because of Timeout value, the Timeout value definitely should not be updated.
If the EFI_INVALID_PARAMETER is because of other parameters, the status should be returned to the caller.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, October 27, 2017 11:44 PM
To: Julien Grall <julien.grall@linaro.org>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; 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

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,
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel