| Message ID | 1427402714-15564-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 March 2015 at 19:08, Laszlo Ersek <lersek@redhat.com> wrote: > 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 <ard.biesheuvel@linaro.org> >> --- >> .../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). > As I said on IRC, it makes no sense to only signal the event channel if we managed to write a couple of characters. I will drop the if there > 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": > I stand corrected. >> >> /** >> 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/
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; }
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 <ard.biesheuvel@linaro.org> --- .../XenConsoleSerialPortLib.c | 24 ++++++++++++---------- 1 file changed, 13 insertions(+), 11 deletions(-)