UART FCR doesn't support flush #83

cperciva · 2022-10-13T17:57:52Z

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 · 2022-10-13T18:13:08Z

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

lauralt · 2022-10-13T18:48:25Z

@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 · 2022-10-13T19:08:40Z

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 · 2022-10-14T08:36:13Z

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 · 2022-10-14T17:30:26Z

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

emaste · 2022-10-14T17:32:16Z

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 · 2022-10-17T06:55:09Z

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 · 2023-03-24T17:44:36Z

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 · 2023-03-24T19:24:39Z

Sounds good to me!

dhriti-rajan · 2023-04-06T20:20:10Z

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 · 2023-04-11T08:48:02Z

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 · 2023-04-11T09:23:17Z

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 · 2023-04-28T05:08:35Z

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 · 2023-05-02T11:55:32Z

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

00xc · 2023-10-12T17:41:10Z

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

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 ¹.
We need to flush the input buffer when requested, plus clear the LSR data ready bit (LSR_DATA_READY_BIT).
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

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 · 2023-10-12T18:44:47Z

@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 · 2023-10-12T20:16:08Z

@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 · 2023-10-12T22:20:52Z

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 <carlos.lopez@suse.com>

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 <carlos.lopezr4096@gmail.com>

andreeaflorescu added enhancement New feature or request help wanted Extra attention is needed labels Oct 17, 2022

00xc linked a pull request Nov 8, 2023 that will close this issue

serial: implement receive FIFO flush via FCR #103

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UART FCR doesn't support flush #83

UART FCR doesn't support flush #83

cperciva commented Oct 13, 2022

cperciva commented Oct 13, 2022

lauralt commented Oct 13, 2022

cperciva commented Oct 13, 2022

andreeaflorescu commented Oct 14, 2022

cperciva commented Oct 14, 2022

emaste commented Oct 14, 2022 •

edited

Loading

andreeaflorescu commented Oct 17, 2022

dhriti-rajan commented Mar 24, 2023

cperciva commented Mar 24, 2023

dhriti-rajan commented Apr 6, 2023

andreeaflorescu commented Apr 11, 2023

andreeaflorescu commented Apr 11, 2023

dhriti-rajan commented Apr 28, 2023

andreeaflorescu commented May 2, 2023

00xc commented Oct 12, 2023 •

edited

Loading

cperciva commented Oct 12, 2023

00xc commented Oct 12, 2023 •

edited

Loading

cperciva commented Oct 12, 2023

UART FCR doesn't support flush #83

UART FCR doesn't support flush #83

Comments

cperciva commented Oct 13, 2022

cperciva commented Oct 13, 2022

lauralt commented Oct 13, 2022

cperciva commented Oct 13, 2022

andreeaflorescu commented Oct 14, 2022

cperciva commented Oct 14, 2022

emaste commented Oct 14, 2022 • edited Loading

andreeaflorescu commented Oct 17, 2022

dhriti-rajan commented Mar 24, 2023

cperciva commented Mar 24, 2023

dhriti-rajan commented Apr 6, 2023

andreeaflorescu commented Apr 11, 2023

andreeaflorescu commented Apr 11, 2023

dhriti-rajan commented Apr 28, 2023

andreeaflorescu commented May 2, 2023

00xc commented Oct 12, 2023 • edited Loading

Footnotes

cperciva commented Oct 12, 2023

00xc commented Oct 12, 2023 • edited Loading

cperciva commented Oct 12, 2023

emaste commented Oct 14, 2022 •

edited

Loading

00xc commented Oct 12, 2023 •

edited

Loading

00xc commented Oct 12, 2023 •

edited

Loading