UART FCR doesn't support flush

[cperciva]

The README file notes that

The Fifo Control Register (FCR) is not emulated; there is no support yet for directly controlling the FIFOs (which, in this implementation, are always enabled).

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.

[cperciva]

FreeBSD patch which works around this issue, for reference: https://reviews.freebsd.org/D36979

[lauralt]

@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.

[cperciva]

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.

[andreeaflorescu]

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.

[cperciva]

I'll add it to my to-do list. For now the workaround I implemented in FreeBSD suffices, so this isn't urgent.

[emaste]

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]

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

I've added the "help wanted" label in case somebody else wants to pick it up to be easier to find.

[dhriti-rajan]

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?

[cperciva]

Sounds good to me!

[dhriti-rajan]

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)?

[andreeaflorescu]

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?

[andreeaflorescu]

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.

[dhriti-rajan]

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.

[andreeaflorescu]

That's ok, thanks for letting us know @dhriti-rajan

[00xc]

Hi, I'm taking a look into this. If I understand correctly, for an initial FCR implementation:

1. We do not need to flush the output buffer, since we immediately flush on write, and LSR_EMPTY_THR_BIT is always set via DEFAULT_LINE_STATUS [^1].
2. We need to flush the input buffer when requested, plus clear the LSR data ready bit (LSR_DATA_READY_BIT).
3. The rest of bits we can safely ignore for now.

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 (UART_LSR_BI in their codebase), but I don't think it's used in vm-superio:

https://github.com/qemu/qemu/blob/a51e5124a655b3dad80b36b18547cb1eca2c5eb2/hw/char/serial.c#L407-L420

[^1]: Keep in mind the FreeBSD patch above was wrong and needed an amending because the Empty Transmitter Holding Register (LSR_EMPTY_THR_BIT, or LSR_TEMT in their patch) is set to zero when the buffer is not empty, and the opposite condition was being checked. See: https://github.com/freebsd/freebsd-src/commit/5ad8c32c722b58da4c153f241201af51b11f3152

[cperciva]

@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.

[00xc]

@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 git apply, and then pointing the firecracker dependency to it, like so:

firecracker/src/vmm $ cargo rm vm-superio
firecracker/src/vmm $ cargo add --path $local_path_to_this_repo/vm-superio

At this point if you run git diff you should see you no longer depend on upstream vm-superio but a local version.

Then you can build and boot normally.

[cperciva]

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.