Re: [Qemu-trivial] [PATCH v3] hw/char/escc: Lower irq when transmit buffer is filled

[Philippe Mathieu-Daudé]

On 5/2/19 11:04 AM, Laurent Vivier wrote:
> On 19/04/2019 17:40, Stephen Checkoway wrote:
>> The SCC/ESCC will briefly stop asserting an interrupt when the
>> transmit FIFO is filled.
>>
>> This code doesn't model the transmit FIFO/shift register so the
>> pending transmit interrupt is never deasserted which means that an
>> edge-triggered interrupt controller will never see the low-to-high
>> transition it needs to raise another interrupt. The practical
>> consequence of this is that guest firmware with an interrupt service
>> routine for the ESCC that does not send all of the data it has
>> immediately will stop sending data if the following sequence of
>> events occurs:
>> 1. Disable processor interrupts
>> 2. Write a character to the ESCC
>> 3. Add additional characters to a buffer which is drained by the ISR
>> 4. Enable processor interrupts
>>
>> In this case, the first character will be sent, the interrupt will
>> fire and the ISR will output the second character. Since the pending
>> transmit interrupt remains asserted, no additional interrupts will
>> ever fire.
>>
>> This behavior was triggered by firmware for an embedded system with a
>> Z85C30 which necessitated this patch.
>>
>> This patch fixes that situation by explicitly lowering the IRQ when a
>> character is written to the buffer and no other interrupts are currently
>> pending.
>>
>> Signed-off-by: Stephen Checkoway <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>
>> I added a sentence about the Z85C30 necessitating this to the commit message.
>>
>>  hw/char/escc.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/char/escc.c b/hw/char/escc.c
>> index 628f5f81f7..c5b05a63f1 100644
>> --- a/hw/char/escc.c
>> +++ b/hw/char/escc.c
>> @@ -509,6 +509,13 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>>          break;
>>      case SERIAL_DATA:
>>          trace_escc_mem_writeb_data(CHN_C(s), val);
>> +        /*
>> +         * Lower the irq when data is written to the Tx buffer and no other
>> +         * interrupts are currently pending. The irq will be raised again 
>> once
>> +         * the Tx buffer becomes empty below.
>> +         */
>> +        s->txint = 0;
>> +        escc_update_irq(s);
>>          s->tx = val;
>>          if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
>>              if (qemu_chr_fe_backend_connected(&s->chr)) {
>>
> 
> 
> Applied to my trivial-patches branch.

Mark, Artyom, are you OK with this patch?

> 
> Paolo, Marc-André, if you disagree with this change or prefer to take it
> through one of your queues, I can remove it from mine. Let me know.
> 
> 
> Thanks,
> Laurent
>