| Message ID | 20190419154041.77802-1-stephen.checkoway@oberlin.edu |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] hw/char/escc: Lower irq when transmit buffer is filled | expand |
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 <stephen.checkoway@oberlin.edu> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > > 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. 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
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 <stephen.checkoway@oberlin.edu> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> >> 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 >
On 02/05/2019 13:11, Philippe Mathieu-Daudé wrote: > 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 <stephen.checkoway@oberlin.edu> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> >>> 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? I started testing this with my OpenBIOS test images at the start of the week, but unfortunately got distracted by real life :) I've now finished and confirmed there are no regressions in my local tests, so I'll include this in the PR I am planning to send shortly containing the leon3 updates. ATB, Mark.
On 03/05/2019 09:14, Mark Cave-Ayland wrote: > On 02/05/2019 13:11, Philippe Mathieu-Daudé wrote: > >> 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 <stephen.checkoway@oberlin.edu> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> >>>> 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? > > I started testing this with my OpenBIOS test images at the start of the week, but > unfortunately got distracted by real life :) > > I've now finished and confirmed there are no regressions in my local tests, so I'll > include this in the PR I am planning to send shortly containing the leon3 updates. Hi Mark, I've already included a leon3 patch ("hw/sparc/leon3: Allow load of uImage firmwares") in the PR I sent yesterday for the trivial branch. I've removed from my PR this patch about the ESCC, so you can take it. Thanks, Laurent
On 03/05/2019 08:21, Laurent Vivier wrote: >>> Mark, Artyom, are you OK with this patch? >> >> I started testing this with my OpenBIOS test images at the start of the week, but >> unfortunately got distracted by real life :) >> >> I've now finished and confirmed there are no regressions in my local tests, so I'll >> include this in the PR I am planning to send shortly containing the leon3 updates. > > Hi Mark, > > I've already included a leon3 patch ("hw/sparc/leon3: Allow load of > uImage firmwares") in the PR I sent yesterday for the trivial branch. > I've removed from my PR this patch about the ESCC, so you can take it. Hi Laurent, Understood, and thanks for looking out for the patch. My first week back has been quite busy so I'm still playing catch-up here... ATB, Mark.
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)) {