On 03/26/15 21:45, Ard Biesheuvel wrote:
> This fixes a particularly nasty interaction between TerminalDxe
> and XenConsoleSerialPortLib where the latter may write fewer
> bytes than it was passed in its ->Write() function, which is
> interpreted by TerminalDxe as a failure condition, causing the
> its driver binding to fail, which in turn causes other drivers
> (the Intel BDS under ArmPlatformPkg) to fail due to a missing
> console pipe.
>
> While one would assume that throttling of the output is the
> responsibility of the console/terminal layer, in this case we
> can work around it by notifying the hypervisor of the partial
> write, and looping until it makes some more room for us.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> .../XenConsoleSerialPortLib.c | 24
> ++++++++++++----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git
> a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> index 467cb27a30c4..9bb2016c2f32 100644
> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> @@ -80,22 +80,24 @@ SerialPortWrite (
> return 0;
> }
>
> - Consumer = mXenConsoleInterface->out_cons;
> - Producer = mXenConsoleInterface->out_prod;
> + Sent = 0;
> + do {
> + Consumer = mXenConsoleInterface->out_cons;
> + Producer = mXenConsoleInterface->out_prod;
>
> - MemoryFence ();
> + MemoryFence ();
>
> - Sent = 0;
> - while (Sent < NumberOfBytes && ((Producer - Consumer) < sizeof
> (mXenConsoleInterface->out)))
> - mXenConsoleInterface->out[MASK_XENCONS_IDX(Producer++,
> mXenConsoleInterface->out)] = Buffer[Sent++];
> + while (Sent < NumberOfBytes && ((Producer - Consumer) < sizeof
> (mXenConsoleInterface->out)))
> + mXenConsoleInterface->out[MASK_XENCONS_IDX(Producer++,
> mXenConsoleInterface->out)] = Buffer[Sent++];
>
> - MemoryFence ();
> + MemoryFence ();
>
> - mXenConsoleInterface->out_prod = Producer;
> + mXenConsoleInterface->out_prod = Producer;
>
> - if (Sent > 0) {
> - XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain);
> - }
> + if (Sent > 0) {
> + XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain);
> + }
> + } while (Sent < NumberOfBytes);
>
> return Sent;
> }
>
The code looks okay to me (while deferring to the Xen guys of course).
However, your remark in the commit message about TerminalDxe and the
expected behavior of SerialPortLibWrite() is not correct. Please consult
the documentation of the library class in
"MdePkg/Include/Library/SerialPortLib.h":
>
> /**
> Write data from buffer to serial device.
>
> Writes NumberOfBytes data bytes from Buffer to the serial device.
> The number of bytes actually written to the serial device is returned.
> If the return value is less than NumberOfBytes, then the write operation
> failed.
> If Buffer is NULL, then ASSERT().
> If NumberOfBytes is zero, then return 0.
>
> @param Buffer Pointer to the data buffer to be written.
> @param NumberOfBytes Number of bytes to written to the serial device.
>
> @retval 0 NumberOfBytes is 0.
> @retval >0 The number of bytes written to the serial device.
> If this value is less than NumberOfBytes, then the
> read operation failed.
>
> **/
> UINTN
> EFIAPI
> SerialPortWrite (
> IN UINT8 *Buffer,
> IN UINTN NumberOfBytes
> );
Correspondingly, in *all* implementations of SerialPortWrite(), flow
control is an internal matter. If you need to spin a few times (or even
many many times) to make progress, you have to.
Compare
"ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c", which
is used in "ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc" (ie. for
real hardware).
It calls PL011UartWrite() in
"ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c", and that function writes
the entire buffer too, handling flow control internally:
> UINTN
> EFIAPI
> PL011UartWrite (
> IN UINTN UartBase,
> IN UINT8 *Buffer,
> IN UINTN NumberOfBytes
> )
> {
> UINT8* CONST Final = &Buffer[NumberOfBytes];
>
> while (Buffer < Final) {
> // Wait until UART able to accept another char
> while ((MmioRead32 (UartBase + UARTFR) & UART_TX_FULL_FLAG_MASK));
>
> MmioWrite8 (UartBase + UARTDR, *Buffer++);
> }
>
> return NumberOfBytes;
> }
Please update the commit message accordingly in the next version.
Thanks!
Laszlo
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
