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

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

[Stephen Checkoway]

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 fixes that situation by explicitly lowering the IRQ when a
character is written to the buffer.

Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
---
 hw/char/escc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 628f5f81f7..bea55ad8da 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -509,6 +509,7 @@ static void escc_mem_write(void *opaque, hwaddr addr,
         break;
     case SERIAL_DATA:
         trace_escc_mem_writeb_data(CHN_C(s), val);
+        qemu_irq_lower(s->irq);
         s->tx = val;
         if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
             if (qemu_chr_fe_backend_connected(&s->chr)) {
-- 
2.17.2 (Apple Git-113)

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

[Paolo Bonzini]

On 05/03/19 06:10, 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 fixes that situation by explicitly lowering the IRQ when a
> character is written to the buffer.
> 
> Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>

Looks good but I would like Mark to give his ack as well.

Mark, could you also add hw/char/escc.c to both SPARC and Mac sections
of MAINTAINERS?

Thanks,

Paolo

> ---
>  hw/char/escc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index 628f5f81f7..bea55ad8da 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -509,6 +509,7 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>          break;
>      case SERIAL_DATA:
>          trace_escc_mem_writeb_data(CHN_C(s), val);
> +        qemu_irq_lower(s->irq);
>          s->tx = val;
>          if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
>              if (qemu_chr_fe_backend_connected(&s->chr)) {
> 

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

[Philippe Mathieu-Daudé]

On 3/6/19 12:01 PM, Paolo Bonzini wrote:
> On 05/03/19 06:10, 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 fixes that situation by explicitly lowering the IRQ when a
>> character is written to the buffer.
>>
>> Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
> 
> Looks good but I would like Mark to give his ack as well.
> 
> Mark, could you also add hw/char/escc.c to both SPARC and Mac sections
> of MAINTAINERS?

Cc'ing Artyom who also made some changes in this file, and Laurent.


Stephen, which particular chipset are you using?

This file models various types. I had a talk with Mark or Laurent at
last KVM forum about this device. IIRC, while the sun4m machines use a
real chipset, the MacIO embeds an slighly modified IP core.

I can't find what you describe in the Z85C30 doc, however in the ESCC
datasheet referenced in escc.c (which happens to be a 85MiB pdf!!!) I found:

  Transmit Interrupts and Transmit Buffer Empty Bit on the NMOS/CMOS

  The TxIP is reset either by writing data to the transmit buffer or
  by issuing the Reset Tx Int command in WR0.

I understand the NMOS/CMOS desc. matches the Z85C30 (no FIFO used).

So your description and patch makes sens.
What worries me is the controller could have other pending IRQs to
deliver and you are clearing them. Shouldn't we only clear the
INTR_TXINT bit, and call escc_update_irq() which should lower the IRQ if
no bits are pending?

Maybe as:

    s->wregs[W_INTR] &= ~INTR_TXINT;
    escc_update_irq(s);

Thanks,

Phil.

> Thanks,
> 
> Paolo
> 
>> ---
>>  hw/char/escc.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/char/escc.c b/hw/char/escc.c
>> index 628f5f81f7..bea55ad8da 100644
>> --- a/hw/char/escc.c
>> +++ b/hw/char/escc.c
>> @@ -509,6 +509,7 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>>          break;
>>      case SERIAL_DATA:
>>          trace_escc_mem_writeb_data(CHN_C(s), val);
>> +        qemu_irq_lower(s->irq);
>>          s->tx = val;
>>          if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
>>              if (qemu_chr_fe_backend_connected(&s->chr)) {
>>
> 
> 

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

[Stephen Checkoway]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]



On Apr 10, 2019, at 16:01, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 3/6/19 12:01 PM, Paolo Bonzini wrote:
>> On 05/03/19 06:10, 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 fixes that situation by explicitly lowering the IRQ when a
>>> character is written to the buffer.
>>> 
>>> Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
>> 
>> Looks good but I would like Mark to give his ack as well.
>> 
>> Mark, could you also add hw/char/escc.c to both SPARC and Mac sections
>> of MAINTAINERS?
> 
> Cc'ing Artyom who also made some changes in this file, and Laurent.
> 
> 
> Stephen, which particular chipset are you using?

I'm away from the hardware, but my notes say Z80C30. I've included the image from the board if you want to try to decipher the rest of the part number. My best guess is it's a Z85C3008VEC. If it's important, I can ask someone who is a few thousand km closer to the hardware than I currently am to take a look.



> This file models various types. I had a talk with Mark or Laurent at
> last KVM forum about this device. IIRC, while the sun4m machines use a
> real chipset, the MacIO embeds an slighly modified IP core.
> 
> I can't find what you describe in the Z85C30 doc, however in the ESCC
> datasheet referenced in escc.c (which happens to be a 85MiB pdf!!!) I found:

(This chip is ridiculously configurable, I honestly found it much easier to write a driver by reverse engineering existing firmware than by reading the part sheet.)

> 
>  Transmit Interrupts and Transmit Buffer Empty Bit on the NMOS/CMOS
> 
>  The TxIP is reset either by writing data to the transmit buffer or
>  by issuing the Reset Tx Int command in WR0.
> 
> I understand the NMOS/CMOS desc. matches the Z85C30 (no FIFO used).

It has been a little while since I was last looking at this, but my recollection was the SCC has a 1-byte transmit buffer and the ESCC has a small (4 maybe?) byte transmit buffer. But in either case writing data to the transmit buffer should clear the interrupt.

> So your description and patch makes sens.
> What worries me is the controller could have other pending IRQs to
> deliver and you are clearing them. Shouldn't we only clear the
> INTR_TXINT bit, and call escc_update_irq() which should lower the IRQ if
> no bits are pending?
> 
> Maybe as:
> 
>    s->wregs[W_INTR] &= ~INTR_TXINT;
>    escc_update_irq(s);

Ah, I think I see. In this case, escc_update_irq() will lower the IRQ if no other interrupts are pending and the set_txint() will raise it again. Otherwise, it'll remain raised. I can give this a shot and see how it goes. (I think this should be R_INTR instead of W_INTR, but I'll double check once I have an opportunity to dig into this again.)

Thanks for looking at this!

Steve


-- 
Stephen Checkoway






[-- Attachment #2.1: Type: text/html, Size: 5653 bytes --]

[-- Attachment #2.2: Z85C300.png --]
[-- Type: image/png, Size: 546254 bytes --]

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

[Stephen Checkoway]

> On Apr 10, 2019, at 16:01, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> So your description and patch makes sens.
> What worries me is the controller could have other pending IRQs to
> deliver and you are clearing them. Shouldn't we only clear the
> INTR_TXINT bit, and call escc_update_irq() which should lower the IRQ if
> no bits are pending?
> 
> Maybe as:
> 
>    s->wregs[W_INTR] &= ~INTR_TXINT;
>    escc_update_irq(s);

I just sent a v2 patch (but it looks like I failed to include Philippe, sorry about that!)

I used

    s->txint = 0;
    escc_update_irq(s);

which appears to do the right thing in my tests.

Thank you,

Steve

-- 
Stephen Checkoway