Re: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead

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

  *Control = 0;
  if (!SerialPortPoll ()) {
    *Control = EFI_SERIAL_INPUT_BUFFER_EMPTY;
  }
  return RETURN_SUCCESS;

As example, the code above (in OvmfPkg\Library\XenConsoleSerialPortLib\XenConsoleSerialPortLib.c) can be simply used in SerialPortGetControl().


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, August 16, 2017 10:02 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead

On 08/16/17 01:59, Kinney, Michael D wrote:
> Laszlo,
> 
> gBS->Stall() layers on top of the Metronome Architectural Protocol.
> 
> armvirtqemu.dsc:  EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
> 
> And this implementation layers on top of a TimerLib
> 
> armvirtqemu.dsc:  
> TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
> 
> There are a couple PCDs involved in this module and lib. Maybe the 
> ArmVirtPkg needs to set some different PCD values to get a more 
> accurate
> gBS->Stall() when running through QEMU.

The issue is different; I checked gBS->Stall() in the above context and it waits for the appropriate time (usually just 1-2 microseconds more than requested).

Instead, the following happens:

- TerminalConInTimerHandler() in
"MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c" calls
SerialIo->GetControl().

- If the GetControl() call fails (for any reason), or the returned Control word has EFI_SERIAL_INPUT_BUFFER_EMPTY clear, then
TerminalConInTimerHandler() enters a loop:

    //
    // Fetch all the keys in the serial buffer,
    // and insert the byte stream into RawFIFO.
    //

- The loop body calls GetOneKeyFromSerial() --> SerialIo->Read(), with a
1 byte buffer.

- The loop runs until the "raw data FIFO buffer" is filled completely
(256 byte is the size, apparently -- RAW_FIFO_MAX_NUMBER), or
GetOneKeyFromSerial() returns an error.

- The SerialPortGetControl() function in "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c"
returns constant RETURN_UNSUPPORTED. According to the library class header "MdePkg/Include/Library/SerialPortLib.h", this is a valid thing to do ("The serial device does not support this operation"). However, it will cause TerminalConInTimerHandler() to *always* enter the loop, even if there is no pending data to read.

- Because the input queue is empty, GetOneKeyFromSerial() will take a full second before it times out.

- And the final piece of the puzzle is that the event associated with
TerminalConInTimerHandler() is signaled at 50Hz (0.02s period, see KEYBOARD_TIMER_INTERVAL), initially. It can dynamically adjust its frequency to the serial device's timeout, but in practice, as soon as the current TerminalConInTimerHandler() frame returns, there's
(apparently) another execution queued already. So basically we're stuck in the timer event handler.

I think we should implement the missing ("unsupported") functions in "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" and possibly in "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c".
I believe we could use
"ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c" as an example.

Thanks
Laszlo