ns8250: Check if flush via FCR succeeded
ClosedPublic
Actions

Authored by cperciva on Oct 13 2022, 6:11 PM.

Details

Reviewers

imp
jrtc27

Commits

rGc4b68e7e53bb: ns8250: Check if flush via FCR succeeded

Summary

The emulated UART in the Firecracker VMM (aka the implementation in the
rust-vmm/vm-superio project) includes FIFOs but does not implement the
FCR register, which is used by ns8250_flush to flush the FIFOs.

Check the LSR to see if there is still data in the FIFOs and call
ns8250_drain if necessary.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 47833
Build 44720: arc lint + arc unit

Event Timeline

cperciva requested review of this revision.Oct 13 2022, 6:11 PM

cperciva created this revision.

Harbormaster completed remote builds in B47833: Diff 111769.Oct 13 2022, 6:11 PM

IMO it's worth putting a link to https://github.com/rust-vmm/vm-superio/issues/83 in here, with the intent that this workaround could eventually be removed again.

emaste added inline comments.Oct 13 2022, 6:39 PM

sys/dev/uart/uart_dev_ns8250.c
232	`if (drain != 0)` may be more style(9)ish

imp added inline comments.Oct 13 2022, 7:08 PM

sys/dev/uart/uart_dev_ns8250.c
214	drain could be a bool since we don't use the bits set for anything.

emaste added inline comments.Oct 13 2022, 7:10 PM

sys/dev/uart/uart_dev_ns8250.c
214	I was about to make that comment before noticing we pass it to ns8250_drain() which does use the bits.

Add comment and change if (drain) to if (drain != 0).

Harbormaster completed remote builds in B47835: Diff 111772.Oct 13 2022, 7:14 PM

Presumably it is possible for a character to arrive between successfully flushing the RX fifo and reading LSR_RXRDY, resulting in a bogus report that UART FCR is broken? IMO that's fine, but an argument for keeping this as a temporary workaround until rust-vmm is fixed.

In D36979#839866, @emaste wrote:

Presumably it is possible for a character to arrive between successfully flushing the RX fifo and reading LSR_RXRDY, resulting in a bogus report that UART FCR is broken? IMO that's fine, but an argument for keeping this as a temporary workaround until rust-vmm is fixed.

Yes, a race condition is possible. Since we're flushing the FIFO anyway I don't think losing a character matters (it would have been lost in the flushing if it arrived a few microseconds earlier) but you're right that theoretically we could get a bogus warning.

I've made a note to remove this once rust-vmm has been fixed (after a reasonable window of time for deployment).

If we see these warnings on !FIRECRACKER systems, we could limit the workaround to FIRECRACKER kernels too.

jrtc27 added inline comments.Oct 13 2022, 9:52 PM

sys/dev/uart/uart_dev_ns8250.c
240	Printing whilst trying to drain seems like an odd thing to do that might cause issues?..

In D36979#839868, @imp wrote:

If we see these warnings on !FIRECRACKER systems, we could limit the workaround to FIRECRACKER kernels too.

Yeah, I considered hiding it behind a machdep.broken_uart_fcr loader setting or similar, but figured that the cost (a single LSR read) was low enough and the rust-vmm bug might surface in other environments. Certainly if the warning shows up outside of Firecracker it's worth investigating.

cperciva added inline comments.Oct 13 2022, 9:54 PM