On Sat, Feb 22, 2025 at 1:27 AM Clément Chigot <chi...@adacore.com> wrote:
>
> On Fri, Feb 14, 2025 at 1:52 PM Clément Chigot <chi...@adacore.com> wrote:
> >
> > Hi Alistair,
> >
> > I've an issue following this patch. When the system is reset (e.g
> > using HTIF syscalls), the fifo might not be empty and thus some
> > characters are lost.
> > I discovered it on a Windows host. But by extending
> > "TX_INTERRUPT_TRIGGER_DELAY_NS" to a huge value, I'm able to reproduce
> > on Linux as well.
>
> The root cause of my issue was unrelated to these early shutdowns. On
> Windows, the character device behind `-serial mon:stdio`
> (char-win-stdio) doesn't provide an `add_watch` method. Therefore,
> `qemu_chr_fe_add_watch` calls always result in an error, flushing the
> fifo. I saw in @Philippe Mathieu-Daudé patch about pl011 that
> `G_SOURCE_CONTINUE` is returned instead of calling it and it does
> work. @Alistair Francis do you remember if there was a reason for
> calling `add_watch` ?
qemu_chr_fe_add_watch() is used in a range of UART devices, I am not
really sure of a different way to write the data out.
I don't see `G_SOURCE_CONTINUE` in pl011.c either
>
> > I've tried to flush within an unrealized function but it didn't work.
> > Any suggestions ?
>
> FTR, I still have found a solution here using
> qemu_register_shutdown_notifier. Though I'm wondering if this is
> useful: the cases where a shutdown occurs between two "fifo_update"
> seems really narrow, but they could happen.
What does the actual hardware do? I don't think it has a shutdown
notifier. I think this is up to the guest to flush the UART.
> @Philippe Mathieu-Daudé AFAICT, the new pl011 and other char devices
> implementing write fifo have the same issue. Thus, pinging you here to
> get your advice.
>
> Thanks,
> Clément
>
> > > static void sifive_uart_reset_enter(Object *obj, ResetType type)
> > > {
> > > ...
> > > + fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
> >
> > I'm also wondering if that part could not lead to memory leak.
> > `fifo8_destroy` is never called and AFAIK, there are ways to reset a
> > device dynamically (e.g snapshot, though not sure if it's supported
> > here).
Good catch, I can send a patch to fix this
Alistair
> >
> > Thanks, Clément
