search

lowRISC / qemu

Fork of QEMU for development of lowRISC platforms (including OpenTitan)
http://www.qemu.org
Other
3 stars 8 forks source link

[ot] Do not drop data on PTY write if it is connected #23

Closed pamaury closed 10 months ago

pamaury commented 1 year ago

The current code in char-pty.c is a bit odd: is uses poll to determine when it can read from a device, but it does not poll for write. Instead, it uses a timer to regularly determine whether the PTY is connected. We can always determine the connection status of a PTY by polling the HUP flag (up for disconnected, down for connected). This commit refactors the code to get rid of the timer and simply poll with no delay on every write to see if it connected.

A better solution would be to use an I/O watch with a callback.

Context: we want to use a PTY to connect the UART to opentitantool. However with the current code, the connection status might take up to a second to be updated so if we want to avoid dropping data from early boot, we need to wait a whole second doing nothing on startup. This will prevent this delay.

rivos-eblot commented 1 year ago

I think you need to discuss this with QEMU upstream. PTY and chardev support is a delicate piece of SW in QEMU, and not breaking stuff might be difficult. You also need to check it works on other Unix flavors (*BSD, macOS, ...) - just trying not opening a can of worms :-)

BTW can't you use sockets to connect the UART?

jwnrt commented 1 year ago

BTW can't you use sockets to connect the UART?

Yes, it just means implementing in-band software flow control manually rather than relying on existing code for serial ports that connect to TTYs.

We've had code mostly working using sockets before, and may go back to using them if this doesn't work out

rivos-eblot commented 1 year ago

BTW can't you use sockets to connect the UART?

Yes, it just means implementing flow control manually rather than relying on existing code for serial ports that connect to TTYs.

Do you need flow control at all in this case?

jwnrt commented 1 year ago

Do you need flow control at all in this case?

I'm not sure, I was fine running the OpenTitan UART tests that I tried, but opentitantool has implemented flow control support for the other "transports" (verilator and FPGAs) so I was thinking there might be some tests that rely on it.

Maybe we can go ahead using sockets without flow control and see if anything breaks.

pamaury commented 1 year ago

@jwnrt thinks it's simpler to reuse the PTY code that we have for real hardware since it's supported by QEMU.

I don't know if we can upstream that change, ideally yes but as you said it's probably quite tedious to check it on every single platform.

rivos-eblot commented 1 year ago

Note that sockets in QEMU are raw sockets so if you need to use out-of-bad data, you will need to provide your own chardev backend. It seems quite some work - not sure it is worth it.

rivos-eblot commented 1 year ago

@jwnrt thinks it's simpler to reuse the PTY code that we have for real hardware since it's supported by QEMU.

I would tend to think it is much simpler to change OT tools than attempting to change anything upstream :-) (personal note: I would never touch this piece of code myself, too many risks of unexpected regressions)

jwnrt commented 1 year ago

Thanks for the guidance @rivos-eblot, I'm going to try shuffle opentitantool around so we can reuse the software flow control code over a socket as well as the PTY

pamaury commented 1 year ago

By some incredible luck, someome posted a very similar patch to the qemu devel mailing list a few days ago: https://lore.kernel.org/qemu-devel/20230816210743.1319018-1-thuth@redhat.com/ I have sent a message saying that we are interested in seeing this change happen.

jwnrt commented 1 year ago

I've updated my opentitantool PR to use files as the UART connection so it can share the verilator code.

If that patch lands in upstream QEMU, maybe we can cherry pick it in and switch to using the serial directly, but for now the file-based connection seems to work fine.

rivos-eblot commented 1 year ago

I've updated my opentitantool PR to use files as the UART connection so it can share the verilator code.

Is this PR https://github.com/lowRISC/opentitan/pull/19471 ?

jwnrt commented 1 year ago

Is this PR lowRISC/opentitan#19471 ?

Yes, that's right.

That PR is based on / waiting on https://github.com/lowRISC/opentitan/pull/19420 which is for having Bazel pass the QEMU flags. We're just missing that flag to ignore the incorrect flashes.

jwnrt commented 10 months ago

By some incredible luck, someome posted a very similar patch to the qemu devel mailing list a few days ago: https://lore.kernel.org/qemu-devel/20230816210743.1319018-1-thuth@redhat.com/ I have sent a message saying that we are interested in seeing this change happen.

That fix landed upstream in (I think) 8.2.0, so maybe we can close this PR and wait until the next rebase on QEMU: https://github.com/qemu/qemu/commit/4f7689f0817a717d18cc8aca298990760f27a89b

jwnrt commented 10 months ago

Apologies, it actually landed pre-8.1.2 so we have the fix in our tree now. I'll close this PR.