UART FCR doesn't support flush #83
Comments
|
FreeBSD patch which works around this issue, for reference: https://reviews.freebsd.org/D36979 |
|
@cperciva thanks for the issue and for the patch link, do you think the fix could be somehow easily adjusted to the serial implementation, or do you have any suggestions for how to fix this? I don't yet have enough context around the register you mentioned. |
|
I think vm-superio needs a few lines of code at https://github.com/rust-vmm/vm-superio/blob/main/crates/vm-superio/src/serial.rs#L541 to handle guest writes to the FCR register. UART registers are a bit weird though -- the FCR register and the IIR register are at the same address depending on the DLAB bit. I don't know how the vm-superio code disambiguates between the two when a write happens. |
|
From what I understand the FCR is write only, while the IIR is read only. That means that even if they use the same address, you can separate between the registers by the operations being called (read or write): https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming#UART_Registers. So far we didn't have a request for implementing this register, and we implemented only the minimum required registers for booting Linux. Would you be interested in submitting the implementation for this register? On our side we unfortunately don't have bandwidth to take care of this request. Alternatively, we can ask folks in the community if they'll like to contribute this. |
|
I'll add it to my to-do list. For now the workaround I implemented in FreeBSD suffices, so this isn't urgent. |
|
It seems like it would make a good project for someone who wanted to get started with rust-vmm if @cperciva doesn't get to it |
andreeaflorescu
added
enhancement
I've added the "help wanted" label in case somebody else wants to pick it up to be easier to find. |
|
Hello! My name's Dhriti, I'm currently taking the CS 360V Virtualization course at UT Austin. One of our projects for the class is to work on an issue in an open-source repository, and since I've been working on learning Rust recently, this repository looked pretty cool. Would it be alright if I worked on this issue? |
|
Sounds good to me! |
|
Sorry for the late follow-up, I've spent a while trying to read up on the issue since I haven't done much with UART before. Is the intent here to get all the tests to pass after removing the patch at https://reviews.freebsd.org/D36979? Also, is the patch located within the codebase for vm-superio (and if so, where)? |
|
Hey @dhriti-rajan, the patch is not located in the same repository. I think it would be nice to be able to test the functionality somehow, but integrating it in Firecracker, and then updating FreeBSD not to use the workaround anymore is a bit too complicated in my opinion. I think it should be outside of the scope of this issue. @cperciva do you think you could help with testing the patch? |
|
As for how to implement this, you can use the documentation of the FCR from here: https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming#UART_Registers. Right now the FIFO is always enabled, and we don't allow writes to FCR to update any of the FIFO related parameters. I'm slightly concerned about how the code might look like when we make the FIFO optional, so it would be great to review some early patches. Looking at what is there to do, I don't think it's necessarily a good first issue for people unfamiliar with virtualization/UART, but feel free to continue to work on it. |
|
Thanks! Sorry for the late reply, I probably won't continue working on it since I'm not sure I'd be able to do it reasonably efficiently. |
|
That's ok, thanks for letting us know @dhriti-rajan |
|
Hi, I'm taking a look into this. If I understand correctly, for an initial FCR implementation:
So something like this should work: diff --git a/vm-superio/src/serial.rs b/vm-superio/src/serial.rs
index 8c30c60..e84dba9 100644
--- a/vm-superio/src/serial.rs
+++ b/vm-superio/src/serial.rs
@@ -23,6 +23,7 @@ use crate::Trigger;
const DATA_OFFSET: u8 = 0;
const IER_OFFSET: u8 = 1;
const IIR_OFFSET: u8 = 2;
+const FCR_OFFSET: u8 = IIR_OFFSET;
const LCR_OFFSET: u8 = 3;
const MCR_OFFSET: u8 = 4;
const LSR_OFFSET: u8 = 5;
@@ -48,6 +49,8 @@ const IIR_NONE_BIT: u8 = 0b0000_0001;
const IIR_THR_EMPTY_BIT: u8 = 0b0000_0010;
const IIR_RDA_BIT: u8 = 0b0000_0100;
+const FCR_FLUSH_IN_BIT: u8 = 0b0000_0010;
+
const LCR_DLAB_BIT: u8 = 0b1000_0000;
const LSR_DATA_READY_BIT: u8 = 0b0000_0001;
@@ -538,7 +541,13 @@ impl<T: Trigger, EV: SerialEvents, W: Write> Serial<T, EV, W> {
LCR_OFFSET => self.line_control = value,
MCR_OFFSET => self.modem_control = value,
SCR_OFFSET => self.scratch = value,
- // We are not interested in writing to other offsets (such as FCR offset).
+ FCR_OFFSET => {
+ // Clear the receive FIFO
+ if value & FCR_FLUSH_IN_BIT != 0 {
+ self.in_buffer.clear();
+ self.clear_lsr_rda_bit();
+ }
+ }
_ => {}
}
Ok(())Thoughts? Am I missing something? I see in the QEMU implementation they also clear the LSR Break Interrupt bit ( Footnotes
|
|
@00xc Looks plausible to me. I'd be happy to test but I'm a rust newbie so I'm not actually sure how to build Firecracker with a patched vm-superio. |
It should be a matter of cloning this repo, applying the patch above via firecracker/src/vmm $ cargo rm vm-superio
firecracker/src/vmm $ cargo add --path $local_path_to_this_repo/vm-superioAt this point if you run Then you can build and boot normally. |
|
Thanks! I've added this to my todo list -- I'm currently a bit backlogged on issues for the upcoming FreeBSD 14.0 but once those are fixed (or if I run out of time to get them fixed before the release) I'll report back. |
The FIFO Control Register (FCR) controls the behavior of the receive and transmit FIFO buffers of the device. The current implementation does not emulate this register, as FIFO buffers are always enabled. However, there are two bits in this register that control flushing of said FIFOS. The transmission FIFO is already flushed by the current implementation on every write, but the receive FIFO is not. This is problematic, as some driver implementations (e.g. FreeBSD's) rely on being able to clear this buffer via the corresponding bit. Implement the correct behavior when a driver sets this bit by clearing `in_buffer`. Since there is no more data in the receive FIFO, the data-ready bit in the Line Status Register (LSR) must be cleared as well, in case it was set. Fixes: rust-vmm#83 Signed-off-by: Carlos López <[email protected]>
The FIFO Control Register (FCR) controls the behavior of the receive and transmit FIFO buffers of the device. The current implementation does not emulate this register, as FIFO buffers are always enabled. However, there are two bits in this register that control flushing of said FIFOS. The transmission FIFO is already flushed by the current implementation on every write, but the receive FIFO is not. This is problematic, as some driver implementations (e.g. FreeBSD's) rely on being able to clear this buffer via the corresponding bit. Implement the correct behavior when a driver sets this bit by clearing `in_buffer`. Since there is no more data in the receive FIFO, the data-ready bit in the Line Status Register (LSR) must be cleared as well, in case it was set. Fixes: rust-vmm#83 Signed-off-by: Carlos López <[email protected]>
The FIFO Control Register (FCR) controls the behavior of the receive and transmit FIFO buffers of the device. The current implementation does not emulate this register, as FIFO buffers are always enabled. However, there are two bits in this register that control flushing of said FIFOS. The transmission FIFO is already flushed by the current implementation on every write, but the receive FIFO is not. This is problematic, as some driver implementations (e.g. FreeBSD's) rely on being able to clear this buffer via the corresponding bit. Implement the correct behavior when a driver sets this bit by clearing `in_buffer`. Since there is no more data in the receive FIFO, the data-ready bit in the Line Status Register (LSR) must be cleared as well, in case it was set. Fixes: rust-vmm#83 Signed-off-by: Carlos López <[email protected]>
The FIFO Control Register (FCR) controls the behavior of the receive and transmit FIFO buffers of the device. The current implementation does not emulate this register, as FIFO buffers are always enabled. However, there are two bits in this register that control flushing of said FIFOS. The transmission FIFO is already flushed by the current implementation on every write, but the receive FIFO is not. This is problematic, as some driver implementations (e.g. FreeBSD's) rely on being able to clear this buffer via the corresponding bit. Implement the correct behavior when a driver sets this bit by clearing `in_buffer`. Since there is no more data in the receive FIFO, the data-ready bit in the Line Status Register (LSR) must be cleared as well, in case it was set. Fixes: rust-vmm#83 Signed-off-by: Carlos López <[email protected]>
The FIFO Control Register (FCR) controls the behavior of the receive and transmit FIFO buffers of the device. The current implementation does not emulate this register, as FIFO buffers are always enabled. However, there are two bits in this register that control flushing of said FIFOS. The transmission FIFO is already flushed by the current implementation on every write, but the receive FIFO is not. This is problematic, as some driver implementations (e.g. FreeBSD's) rely on being able to clear this buffer via the corresponding bit. Implement the correct behavior when a driver sets this bit by clearing `in_buffer`. Since there is no more data in the receive FIFO, the data-ready bit in the Line Status Register (LSR) must be cleared as well, in case it was set. Fixes: rust-vmm#83 Signed-off-by: Carlos López <[email protected]>
The README file notes that
but the Enabled bit (0x1) is not the only thing in the FCR; it also has bits for flushing the receive and transmit FIFOs (0x2, 0x4). FreeBSD makes use of these; for now I'm adding a workaround which checks if the flush was successful and manually drains the FIFO instead if not, but it would be great if this could be fixed in vm-superio.
The text was updated successfully, but these errors were encountered: