On Mon, Feb 24, 2025 at 5:38 AM Alistair Francis <alistai...@gmail.com> wrote:
>
> 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
The patch has not yet been merged to master AFAICT. But here is the
code replacing `qemu_chr_fe_add_watch` in `*_xmit`:
+ if (!fifo8_is_empty(&s->xmit_fifo)) {
+ /* Reschedule another transmission if we couldn't transmit all. */
+ return G_SOURCE_CONTINUE;
+ }
However, it seems that Peter has detected some issues with those
patches (see [2]). So, I'll wait for the patch to land on master
before anything.
[1] https://mail.gnu.org/archive/html/qemu-devel/2025-02/msg01637.html
[2] https://mail.gnu.org/archive/html/qemu-devel/2025-02/msg04209.html
> > > 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.
I don't have access to real hardware. But the reference manual doesn't
mention any shutdown notifier. So I think you're right.
> > @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
