Xen Console input very slow in recent UEFI

Xen Console input very slow in recent UEFI

[Julien Grall]

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?

Cheers,

-- 
Julien Grall

Re: Xen Console input very slow in recent UEFI

[Laszlo Ersek]

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@redhat.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.

... This is why we need active Xen participants in edk2 ;)

Thanks!
Laszlo

Re: Xen Console input very slow in recent UEFI

[Laszlo Ersek]

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@redhat.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?

Thanks,
Laszlo


> 
> ... This is why we need active Xen participants in edk2 ;)
> 
> Thanks!
> Laszlo
> 

Re: Xen Console input very slow in recent UEFI

[Julien Grall]

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@redhat.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

Re: Xen Console input very slow in recent UEFI

[Zeng, Star]

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.

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

Re: Xen Console input very slow in recent UEFI

[Laszlo Ersek]

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
> 

Re: Xen Console input very slow in recent UEFI

[Julien Grall]

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

Re: Xen Console input very slow in recent UEFI

[Laszlo Ersek]

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

Re: Xen Console input very slow in recent UEFI

[Zeng, Star]

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

Re: Xen Console input very slow in recent UEFI

[Julien Grall]

Hi Zeng,

On 30 October 2017 at 01:09, Zeng, Star <star.zeng@intel.com> wrote:
> 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.

Fair enough. Are you going to resend the change you suggest with a
commit message?

Cheers,

-- 
Julien Grall

Re: Xen Console input very slow in recent UEFI

[Zeng, Star]

Patch has been just sent at https://lists.01.org/pipermail/edk2-devel/2017-November/016837.html.

Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Wednesday, November 1, 2017 8:47 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>; heyi.guo@linaro.org; Ni, Ruiyu <ruiyu.ni@intel.com>; 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 Zeng,

On 30 October 2017 at 01:09, Zeng, Star <star.zeng@intel.com> wrote:
> 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.

Fair enough. Are you going to resend the change you suggest with a commit message?

Cheers,

--
Julien Grall