[PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Star Zeng]

https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
reported "Xen Console input very slow in recent UEFI" that appears
after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
SerialDxe: Process timeout consistently in SerialRead".

Julien 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

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 1000000 (1s), "slow down" will be
observed.

SerialPortLib instance can be enhanced 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.

And SerialDxe can also be enhanced like this patch to be more robust
to handle Timeout change.

Cc: Julien Grall <julien.grall@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Compare against the original parameters
  Suggested-by: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>

V2: Compare against the original parameters
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 45 +++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index ebcd92726314..5be77e7acfb0 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -280,12 +280,51 @@ SerialSetAttributes (
   IN EFI_STOP_BITS_TYPE     StopBits
   )
 {
-  EFI_STATUS    Status;
-  EFI_TPL       Tpl;
+  EFI_STATUS                Status;
+  EFI_TPL                   Tpl;
+  UINT64                    OriginalBaudRate;
+  UINT32                    OriginalReceiveFifoDepth;
+  UINT32                    OriginalTimeout;
+  EFI_PARITY_TYPE           OriginalParity;
+  UINT8                     OriginalDataBits;
+  EFI_STOP_BITS_TYPE        OriginalStopBits;
 
+  //
+  // Preserve the original input values in case
+  // SerialPortSetAttributes() updates the input/output parameters even on error.
+  //
+  OriginalBaudRate = BaudRate;
+  OriginalReceiveFifoDepth = ReceiveFifoDepth;
+  OriginalTimeout = Timeout;
+  OriginalParity = Parity;
+  OriginalDataBits = DataBits;
+  OriginalStopBits = StopBits;
   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          != OriginalTimeout) &&
+        (This->Mode->ReceiveFifoDepth == OriginalReceiveFifoDepth) &&
+        (This->Mode->BaudRate         == OriginalBaudRate) &&
+        (This->Mode->DataBits         == (UINT32) OriginalDataBits) &&
+        (This->Mode->Parity           == (UINT32) OriginalParity) &&
+        (This->Mode->StopBits         == (UINT32) OriginalStopBits)) {
+      //
+      // Restore to the original input values.
+      //
+      BaudRate = OriginalBaudRate;
+      ReceiveFifoDepth = OriginalReceiveFifoDepth;
+      Timeout = OriginalTimeout;
+      Parity = OriginalParity;
+      DataBits = OriginalDataBits;
+      StopBits = OriginalStopBits;
+      Status = EFI_SUCCESS;
+    } else {
+      return Status;
+    }
   }
 
   //
-- 
2.7.0.windows.1

Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Laszlo Ersek]

On 11/07/17 02:36, Star Zeng wrote:
> https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
> reported "Xen Console input very slow in recent UEFI" that appears
> after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
> SerialDxe: Process timeout consistently in SerialRead".
> 
> Julien 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
> 
> 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 1000000 (1s), "slow down" will be
> observed.
> 
> SerialPortLib instance can be enhanced 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.
> 
> And SerialDxe can also be enhanced like this patch to be more robust
> to handle Timeout change.
> 
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Compare against the original parameters
>   Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> 
> V2: Compare against the original parameters
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 45 +++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index ebcd92726314..5be77e7acfb0 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -280,12 +280,51 @@ SerialSetAttributes (
>    IN EFI_STOP_BITS_TYPE     StopBits
>    )
>  {
> -  EFI_STATUS    Status;
> -  EFI_TPL       Tpl;
> +  EFI_STATUS                Status;
> +  EFI_TPL                   Tpl;
> +  UINT64                    OriginalBaudRate;
> +  UINT32                    OriginalReceiveFifoDepth;
> +  UINT32                    OriginalTimeout;
> +  EFI_PARITY_TYPE           OriginalParity;
> +  UINT8                     OriginalDataBits;
> +  EFI_STOP_BITS_TYPE        OriginalStopBits;
>  
> +  //
> +  // Preserve the original input values in case
> +  // SerialPortSetAttributes() updates the input/output parameters even on error.
> +  //
> +  OriginalBaudRate = BaudRate;
> +  OriginalReceiveFifoDepth = ReceiveFifoDepth;
> +  OriginalTimeout = Timeout;
> +  OriginalParity = Parity;
> +  OriginalDataBits = DataBits;
> +  OriginalStopBits = StopBits;
>    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          != OriginalTimeout) &&
> +        (This->Mode->ReceiveFifoDepth == OriginalReceiveFifoDepth) &&
> +        (This->Mode->BaudRate         == OriginalBaudRate) &&
> +        (This->Mode->DataBits         == (UINT32) OriginalDataBits) &&
> +        (This->Mode->Parity           == (UINT32) OriginalParity) &&
> +        (This->Mode->StopBits         == (UINT32) OriginalStopBits)) {
> +      //
> +      // Restore to the original input values.
> +      //
> +      BaudRate = OriginalBaudRate;
> +      ReceiveFifoDepth = OriginalReceiveFifoDepth;
> +      Timeout = OriginalTimeout;
> +      Parity = OriginalParity;
> +      DataBits = OriginalDataBits;
> +      StopBits = OriginalStopBits;
> +      Status = EFI_SUCCESS;
> +    } else {
> +      return Status;
> +    }
>    }
>  
>    //
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Laszlo Ersek]

oops, forgot to write down my only comment:

On 11/07/17 19:27, Laszlo Ersek wrote:
> On 11/07/17 02:36, Star Zeng wrote:
>> https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
>> reported "Xen Console input very slow in recent UEFI" that appears
>> after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
>> SerialDxe: Process timeout consistently in SerialRead".
>>
>> Julien 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
>>
>> 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 1000000 (1s), "slow down" will be
>> observed.
>>
>> SerialPortLib instance can be enhanced 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.
>>
>> And SerialDxe can also be enhanced like this patch to be more robust
>> to handle Timeout change.
>>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Compare against the original parameters
>>   Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>>
>> V2: Compare against the original parameters
>> ---
>>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 45 +++++++++++++++++++++++++++--
>>  1 file changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
>> index ebcd92726314..5be77e7acfb0 100644
>> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
>> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
>> @@ -280,12 +280,51 @@ SerialSetAttributes (
>>    IN EFI_STOP_BITS_TYPE     StopBits
>>    )
>>  {
>> -  EFI_STATUS    Status;
>> -  EFI_TPL       Tpl;
>> +  EFI_STATUS                Status;
>> +  EFI_TPL                   Tpl;
>> +  UINT64                    OriginalBaudRate;
>> +  UINT32                    OriginalReceiveFifoDepth;
>> +  UINT32                    OriginalTimeout;
>> +  EFI_PARITY_TYPE           OriginalParity;
>> +  UINT8                     OriginalDataBits;
>> +  EFI_STOP_BITS_TYPE        OriginalStopBits;
>>  
>> +  //
>> +  // Preserve the original input values in case
>> +  // SerialPortSetAttributes() updates the input/output parameters even on error.

This line is 81 characters long; for better edk2 style conformance,
please consider wrapping it, before you push the patch.

Thanks!
Laszlo

>> +  //
>> +  OriginalBaudRate = BaudRate;
>> +  OriginalReceiveFifoDepth = ReceiveFifoDepth;
>> +  OriginalTimeout = Timeout;
>> +  OriginalParity = Parity;
>> +  OriginalDataBits = DataBits;
>> +  OriginalStopBits = StopBits;
>>    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          != OriginalTimeout) &&
>> +        (This->Mode->ReceiveFifoDepth == OriginalReceiveFifoDepth) &&
>> +        (This->Mode->BaudRate         == OriginalBaudRate) &&
>> +        (This->Mode->DataBits         == (UINT32) OriginalDataBits) &&
>> +        (This->Mode->Parity           == (UINT32) OriginalParity) &&
>> +        (This->Mode->StopBits         == (UINT32) OriginalStopBits)) {
>> +      //
>> +      // Restore to the original input values.
>> +      //
>> +      BaudRate = OriginalBaudRate;
>> +      ReceiveFifoDepth = OriginalReceiveFifoDepth;
>> +      Timeout = OriginalTimeout;
>> +      Parity = OriginalParity;
>> +      DataBits = OriginalDataBits;
>> +      StopBits = OriginalStopBits;
>> +      Status = EFI_SUCCESS;
>> +    } else {
>> +      return Status;
>> +    }
>>    }
>>  
>>    //
>>
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> Laszlo
> 

Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Julien Grall]

Hi Star,

On 07/11/17 01:36, Star Zeng wrote:
> https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
> reported "Xen Console input very slow in recent UEFI" that appears
> after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
> SerialDxe: Process timeout consistently in SerialRead".
> 
> Julien 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
> 
> 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 1000000 (1s), "slow down" will be
> observed.
> 
> SerialPortLib instance can be enhanced 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.
> 
> And SerialDxe can also be enhanced like this patch to be more robust
> to handle Timeout change.
> 
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Compare against the original parameters
>    Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>

Tested-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Zeng, Star]

Pushed at a7fd8452964c1a6ffeee1fe07537cb900c0ccb07.
Thanks for review and test.

Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Wednesday, November 8, 2017 10:36 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

Hi Star,

On 07/11/17 01:36, Star Zeng wrote:
> https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
> reported "Xen Console input very slow in recent UEFI" that appears 
> after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
> SerialDxe: Process timeout consistently in SerialRead".
> 
> Julien 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
> 
> 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 1000000 (1s), "slow down" will be 
> observed.
> 
> SerialPortLib instance can be enhanced 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.
> 
> And SerialDxe can also be enhanced like this patch to be more robust 
> to handle Timeout change.
> 
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Compare against the original parameters
>    Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>

Tested-by: Julien Grall <julien.grall@linaro.org>

Cheers,

--
Julien Grall