| Message ID | 1427482577-1467-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 March 2015 at 22:42, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/27/15 19:56, Ard Biesheuvel wrote: >> It is the responsibility of the SerialPortLib implementation >> to deal with flow control if the underlying medium cannot keep >> up with the inflow of data. >> >> So in our SerialPortWrite () function, we should spin as long >> as we need to in order to deliver all the data instead of giving >> up and returning a smaller value than the number of bytes we were >> given. Also, remove the 'if (Sent > 0)' condition on the signalling >> of the event channel: if the buffer is full and we haven't been able >> to add any more data, it makes perfect sense to signal the event >> channel again, even if we have done so before when we did write >> the data. >> >> Also, this patch brings the implementation of XenSerialPortLib >> in sync with the library class documentation. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 61 insertions(+), 24 deletions(-) >> >> diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> index 467cb27a30c4..822e50e7fb2e 100644 >> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> @@ -17,6 +17,7 @@ >> #include <Uefi/UefiBaseType.h> >> >> #include <Library/BaseLib.h> >> +#include <Library/DebugLib.h> >> #include <Library/SerialPortLib.h> >> #include <Library/XenHypercallLib.h> >> >> @@ -34,6 +35,17 @@ >> STATIC evtchn_send_t mXenConsoleEventChain; >> STATIC struct xencons_interface *mXenConsoleInterface; >> >> +/** >> + Initialize the serial device hardware. >> + >> + If no initialization is required, then return RETURN_SUCCESS. >> + If the serial device was successfully initialized, then return RETURN_SUCCESS. >> + If the serial device could not be initialized, then return RETURN_DEVICE_ERROR. >> + >> + @retval RETURN_SUCCESS The serial device was initialized. >> + @retval RETURN_DEVICE_ERROR The serial device could not be initialized. >> + >> +**/ >> RETURN_STATUS >> EFIAPI >> SerialPortInitialize ( >> @@ -41,7 +53,7 @@ SerialPortInitialize ( >> ) >> { >> if (! XenHypercallIsAvailable ()) { >> - return RETURN_NOT_FOUND; >> + return RETURN_DEVICE_ERROR; >> } >> >> if (!mXenConsoleInterface) { >> @@ -57,13 +69,20 @@ SerialPortInitialize ( >> } >> >> /** >> - Write data to serial device. >> + 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 Point of data buffer which need to be written. >> - @param NumberOfBytes Number of output bytes which are cached in Buffer. >> + @param Buffer Pointer to the data buffer to be written. >> + @param NumberOfBytes Number of bytes to written to the serial device. >> >> - @retval 0 Write data failed. >> - @retval !0 Actual number of bytes written to 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 write operation failed. >> >> **/ >> UINTN >> @@ -76,38 +95,50 @@ SerialPortWrite ( >> XENCONS_RING_IDX Consumer, Producer; >> UINTN Sent; >> >> + ASSERT (Buffer != NULL); >> + >> + if (NumberOfBytes == 0) { >> + return 0; >> + } >> + >> if (!mXenConsoleInterface) { >> 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); >> - } >> + >> + } while (Sent < NumberOfBytes); >> >> return Sent; >> } >> >> /** >> - Read data from serial device and save the data in buffer. >> + Read data from serial device and save the datas in buffer. >> >> - @param Buffer Point of data buffer which need to be written. >> - @param NumberOfBytes Size of Buffer[]. >> + Reads NumberOfBytes data bytes from a serial device into the buffer >> + specified by Buffer. The number of bytes actually read is returned. >> + If Buffer is NULL, then ASSERT(). >> + If NumberOfBytes is zero, then return 0. >> >> - @retval 0 Read data failed. >> - @retval !0 Actual number of bytes read from serial device. >> + @param Buffer Pointer to the data buffer to store the data read from the serial device. >> + @param NumberOfBytes Number of bytes which will be read. >> + >> + @retval 0 Read data failed, no data is to be read. >> + @retval >0 Actual number of bytes read from serial device. >> >> **/ >> UINTN >> @@ -120,6 +151,12 @@ SerialPortRead ( >> XENCONS_RING_IDX Consumer, Producer; >> UINTN Received; >> >> + ASSERT (Buffer != NULL); >> + >> + if (NumberOfBytes == 0) { >> + return 0; >> + } >> + >> if (!mXenConsoleInterface) { >> return 0; >> } >> @@ -143,10 +180,10 @@ SerialPortRead ( >> } >> >> /** >> - Check to see if any data is available to be read from the debug device. >> + Polls a serial device to see if there is any data waiting to be read. >> >> - @retval TRUE At least one byte of data is available to be read >> - @retval FALSE No data is available to be read >> + @retval TRUE Data is waiting to be read from the serial device. >> + @retval FALSE There is no data waiting to be read from the serial device. >> >> **/ >> BOOLEAN >> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > I committed your patch to SVN as r17079, with a couple of minimal > tweaks: > Thanks > (1) I prefixed the subject line with "OvmfPkg: ". > > (2) In order to comply with the *letter* :) of > "MdePkg/Include/Library/SerialPortLib.h", you added some ASSERT()s, and > for those you also included <Library/DebugLib.h>. > > Since I'm an unsufferable pedant, I wanted to spell out the DebugLib insufferable > dependency in the sibling INF file as well. Predictably, that triggered > the following (all too well known) error, when I build-tested > ArmVirtualizationXen: > >> .../ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationXen.dsc(...): >> error F002: Library [.../MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf] with constructors has a cycle >> consumed by .../OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf > Yep, tried that as well :-) > So, rather than adding DebugLib to the INF file as well, under > [LibraryClasses], I dropped the DebugLib include, and open-coded the > ASSERT(). This is exactly what we did in > "ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationDxeHobLib/HobLib.c", > in git commit ad90df8a. In fact I copied the macro definition from > there: > >> diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> index 822e50e..4ccb38d 100644 >> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> @@ -17,7 +17,6 @@ >> #include <Uefi/UefiBaseType.h> >> >> #include <Library/BaseLib.h> >> -#include <Library/DebugLib.h> >> #include <Library/SerialPortLib.h> >> #include <Library/XenHypercallLib.h> >> >> @@ -26,6 +25,17 @@ >> #include <IndustryStandard/Xen/event_channel.h> >> >> // >> +// We can't use DebugLib due to a constructor dependency cycle between DebugLib >> +// and ourselves. >> +// >> +#define ASSERT(Expression) \ >> + do { \ >> + if (!(Expression)) { \ >> + CpuDeadLoop (); \ >> + } \ >> + } while (FALSE) >> + >> +// >> // The code below expects these global variables to be mutable, even in the case >> // that we have been incorporated into SEC or PEIM phase modules (which is >> // allowed by our INF description). While this is a dangerous assumption to make > > (3) This is not a tweak, just a note. From a quick look, the > SerialPortRead() function seems to suffer from the same issue. (It is > probably never exposed because callers presumably only read 1 byte at a > time, and even that should be preceded by a call to SerialPortPoll().) > > If you want to fix this, that can be done in a separate patch; this one > precisely states in the commit message that it only deals with output. > The comment suggests that only returning 0 is considered a failure, and returning fewer bytes than there is space available is not. Since Poll () returning TRUE suggests at least 1 byte available, i think it does not make sense to spin @retval 0 Read data failed, no data is to be read. @retval >0 Actual number of bytes read from serial device. ------------------------------------------------------------------------------ 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..822e50e7fb2e 100644 --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c @@ -17,6 +17,7 @@ #include <Uefi/UefiBaseType.h> #include <Library/BaseLib.h> +#include <Library/DebugLib.h> #include <Library/SerialPortLib.h> #include <Library/XenHypercallLib.h> @@ -34,6 +35,17 @@ STATIC evtchn_send_t mXenConsoleEventChain; STATIC struct xencons_interface *mXenConsoleInterface; +/** + Initialize the serial device hardware. + + If no initialization is required, then return RETURN_SUCCESS. + If the serial device was successfully initialized, then return RETURN_SUCCESS. + If the serial device could not be initialized, then return RETURN_DEVICE_ERROR. + + @retval RETURN_SUCCESS The serial device was initialized. + @retval RETURN_DEVICE_ERROR The serial device could not be initialized. + +**/ RETURN_STATUS EFIAPI SerialPortInitialize ( @@ -41,7 +53,7 @@ SerialPortInitialize ( ) { if (! XenHypercallIsAvailable ()) { - return RETURN_NOT_FOUND; + return RETURN_DEVICE_ERROR; } if (!mXenConsoleInterface) { @@ -57,13 +69,20 @@ SerialPortInitialize ( } /** - Write data to serial device. + 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 Point of data buffer which need to be written. - @param NumberOfBytes Number of output bytes which are cached in Buffer. + @param Buffer Pointer to the data buffer to be written. + @param NumberOfBytes Number of bytes to written to the serial device. - @retval 0 Write data failed. - @retval !0 Actual number of bytes written to 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 write operation failed. **/ UINTN @@ -76,38 +95,50 @@ SerialPortWrite ( XENCONS_RING_IDX Consumer, Producer; UINTN Sent; + ASSERT (Buffer != NULL); + + if (NumberOfBytes == 0) { + return 0; + } + if (!mXenConsoleInterface) { 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); - } + + } while (Sent < NumberOfBytes); return Sent; } /** - Read data from serial device and save the data in buffer. + Read data from serial device and save the datas in buffer. - @param Buffer Point of data buffer which need to be written. - @param NumberOfBytes Size of Buffer[]. + Reads NumberOfBytes data bytes from a serial device into the buffer + specified by Buffer. The number of bytes actually read is returned. + If Buffer is NULL, then ASSERT(). + If NumberOfBytes is zero, then return 0. - @retval 0 Read data failed. - @retval !0 Actual number of bytes read from serial device. + @param Buffer Pointer to the data buffer to store the data read from the serial device. + @param NumberOfBytes Number of bytes which will be read. + + @retval 0 Read data failed, no data is to be read. + @retval >0 Actual number of bytes read from serial device. **/ UINTN @@ -120,6 +151,12 @@ SerialPortRead ( XENCONS_RING_IDX Consumer, Producer; UINTN Received; + ASSERT (Buffer != NULL); + + if (NumberOfBytes == 0) { + return 0; + } + if (!mXenConsoleInterface) { return 0; } @@ -143,10 +180,10 @@ SerialPortRead ( } /** - Check to see if any data is available to be read from the debug device. + Polls a serial device to see if there is any data waiting to be read. - @retval TRUE At least one byte of data is available to be read - @retval FALSE No data is available to be read + @retval TRUE Data is waiting to be read from the serial device. + @retval FALSE There is no data waiting to be read from the serial device. **/ BOOLEAN
It is the responsibility of the SerialPortLib implementation to deal with flow control if the underlying medium cannot keep up with the inflow of data. So in our SerialPortWrite () function, we should spin as long as we need to in order to deliver all the data instead of giving up and returning a smaller value than the number of bytes we were given. Also, remove the 'if (Sent > 0)' condition on the signalling of the event channel: if the buffer is full and we haven't been able to add any more data, it makes perfect sense to signal the event channel again, even if we have done so before when we did write the data. Also, this patch brings the implementation of XenSerialPortLib in sync with the library class documentation. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 61 insertions(+), 24 deletions(-)