|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 2/2] arm/xen: vpl011: Fix SBSA UART interrupt assertion
On Tue, 24 Oct 2017, Andre Przywara wrote:
> From: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
>
> With the current SBSA UART emulation, streaming larger amounts of data
> (as caused by "find /", for instance) can lead to character loses.
^ losses
> This is due to the OUT ring buffer getting full, because we change the
> TX interrupt bit only when the FIFO is actually full, and not already
> when it's half-way filled, as the Linux driver expects.
> The SBSA spec does not explicitly state this, but we assume that an
> SBSA compliant UART uses the PL011 default "interrupt FIFO level select
> register" value of "1/2 way". The Linux driver certainly makes this
> assumption, so it expect to be able to write a number of characters
> after the TX interrupt line has been asserted.
> On a similar issue we have the same wrong behaviour on the receive side.
> However changing the RX interrupt to trigger on reaching half of the FIFO
> level will lead to lag, because the guest would not be notified of incoming
> characters until the FIFO is half way filled. This leads to inacceptible
> lags when typing on a terminal.
> Real hardware solves this issue by using the "receive timeout
> interrupt" (RTI), which is triggered when character reception stops for
> 32 baud cycles. As we cannot and do not want to emulate any timing here,
> we slightly abuse the timeout interrupt to notify the guest of new
> characters: when a new character comes in, the RTI is asserted, when
> the FIFO is cleared, the interrupt gets cleared.
>
> So this patch changes the emulated interrupt trigger behaviour to come
> as close to real hardware as possible: the RX and TX interrupt trigger
> when the FIFO gets half full / half empty, and the RTI interrupt signals
> new incoming characters.
>
> [Andre: reword commit message, introduce receive timeout interrupt, add
> comments]
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
This is good, only minor cosmetic comments.
Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Julien, can we have the release-ack?
> ---
> xen/arch/arm/vpl011.c | 131
> ++++++++++++++++++++++++++++++-------------
> xen/include/asm-arm/vpl011.h | 2 +
> 2 files changed, 94 insertions(+), 39 deletions(-)
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 0b0743679f..6d02406acf 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -18,6 +18,9 @@
>
> #define XEN_WANT_FLEX_CONSOLE_RING 1
>
> +/* We assume the PL011 default of "1/2 way" for the FIFO trigger level. */
> +#define SBSA_UART_FIFO_LEVEL (SBSA_UART_FIFO_SIZE / 2)
> +
> #include <xen/errno.h>
> #include <xen/event.h>
> #include <xen/guest_access.h>
> @@ -93,24 +96,37 @@ static uint8_t vpl011_read_data(struct domain *d)
> */
> if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
> {
> + unsigned int fifo_level;
> +
> data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
> in_cons += 1;
> smp_mb();
> intf->in_cons = in_cons;
> +
> + fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
This is only cosmetics but can we call xencons_queued only once? See the `if'
just above.
> + /* If the FIFO is now empty, we clear the receive timeout interrupt.
> */
> + if ( fifo_level == 0 )
> + {
> + vpl011->uartfr |= RXFE;
> + vpl011->uartris &= ~RTI;
> + }
> +
> + /* If the FIFO is more than half empty, we clear the RX interrupt. */
> + if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
> + vpl011->uartris &= ~RXI;
> +
> + vpl011_update_interrupt_status(d);
> }
> else
> gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>
> - if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
> - {
> - vpl011->uartfr |= RXFE;
> - vpl011->uartris &= ~RXI;
> - }
> -
> + /*
> + * We have consumed a character or the FIFO was empty, so clear the
> + * "FIFO full" bit.
> + */
> vpl011->uartfr &= ~RXFF;
>
> - vpl011_update_interrupt_status(d);
> -
> VPL011_UNLOCK(d, flags);
>
> /*
> @@ -122,6 +138,24 @@ static uint8_t vpl011_read_data(struct domain *d)
> return data;
> }
>
> +static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
> + unsigned int fifo_level)
> +{
> + struct xencons_interface *intf = vpl011->ring_buf;
> + unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_LEVEL;
> +
> + BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
> +
> + /*
> + * Set the TXI bit only when there is space for fifo_size/2 bytes which
> + * is the trigger level for asserting/de-assterting the TX interrupt.
> + */
> + if ( fifo_level <= fifo_threshold )
> + vpl011->uartris |= TXI;
> + else
> + vpl011->uartris &= ~TXI;
> +}
> +
> static void vpl011_write_data(struct domain *d, uint8_t data)
> {
> unsigned long flags;
> @@ -146,33 +180,37 @@ static void vpl011_write_data(struct domain *d, uint8_t
> data)
> if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
> sizeof (intf->out) )
> {
> + unsigned int fifo_level;
> +
> intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
> out_prod += 1;
> smp_wmb();
> intf->out_prod = out_prod;
> - }
> - else
> - gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>
> - if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
> - sizeof (intf->out) )
> - {
> - vpl011->uartfr |= TXFF;
> - vpl011->uartris &= ~TXI;
> + fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));
Same here
> - /*
> - * This bit is set only when FIFO becomes full. This ensures that
> - * the SBSA UART driver can write the early console data as fast as
> - * possible, without waiting for the BUSY bit to get cleared before
> - * writing each byte.
> - */
> - vpl011->uartfr |= BUSY;
> + if ( fifo_level == sizeof (intf->out) )
code style
> + {
> + vpl011->uartfr |= TXFF;
> +
> + /*
> + * This bit is set only when FIFO becomes full. This ensures that
> + * the SBSA UART driver can write the early console data as fast
> as
> + * possible, without waiting for the BUSY bit to get cleared
> before
> + * writing each byte.
> + */
> + vpl011->uartfr |= BUSY;
> + }
> +
> + vpl011_update_tx_fifo_status(vpl011, fifo_level);
> +
> + vpl011_update_interrupt_status(d);
> }
> + else
> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>
> vpl011->uartfr &= ~TXFE;
>
> - vpl011_update_interrupt_status(d);
> -
> VPL011_UNLOCK(d, flags);
>
> /*
> @@ -344,7 +382,7 @@ static void vpl011_data_avail(struct domain *d)
> struct vpl011 *vpl011 = &d->arch.vpl011;
> struct xencons_interface *intf = vpl011->ring_buf;
> XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
> - XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
> + XENCONS_RING_IDX in_fifo_level, out_fifo_level;
>
> VPL011_LOCK(d, flags);
>
> @@ -355,28 +393,41 @@ static void vpl011_data_avail(struct domain *d)
>
> smp_rmb();
>
> - in_ring_qsize = xencons_queued(in_prod,
> + in_fifo_level = xencons_queued(in_prod,
> in_cons,
> sizeof(intf->in));
>
> - out_ring_qsize = xencons_queued(out_prod,
> + out_fifo_level = xencons_queued(out_prod,
> out_cons,
> sizeof(intf->out));
>
> - /* Update the uart rx state if the buffer is not empty. */
> - if ( in_ring_qsize != 0 )
> - {
> + /**** Update the UART RX state ****/
> +
> + /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
> + if ( in_fifo_level > 0 )
> vpl011->uartfr &= ~RXFE;
> - if ( in_ring_qsize == sizeof(intf->in) )
> - vpl011->uartfr |= RXFF;
> +
> + /* Set the FIFO_FULL bit if the Xen buffer is full. */
> + if ( in_fifo_level == sizeof(intf->in) )
> + vpl011->uartfr |= RXFF;
> +
> + /* Assert the RX interrupt if the FIFO is more than half way filled. */
> + if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
> vpl011->uartris |= RXI;
> - }
>
> - /* Update the uart tx state if the buffer is not full. */
> - if ( out_ring_qsize != sizeof(intf->out) )
> + /*
> + * If the input queue is not empty, we assert the receive timeout
> interrupt.
> + * As we don't emulate any timing here, so we ignore the actual timeout
> + * of 32 baud cycles.
> + */
> + if ( in_fifo_level > 0 )
> + vpl011->uartris |= RTI;
> +
> + /**** Update the UART TX state ****/
> +
> + if ( out_fifo_level != sizeof(intf->out) )
> {
> vpl011->uartfr &= ~TXFF;
> - vpl011->uartris |= TXI;
>
> /*
> * Clear the BUSY bit as soon as space becomes available
> @@ -385,12 +436,14 @@ static void vpl011_data_avail(struct domain *d)
> */
> vpl011->uartfr &= ~BUSY;
>
> - if ( out_ring_qsize == 0 )
> - vpl011->uartfr |= TXFE;
> + vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
> }
>
> vpl011_update_interrupt_status(d);
>
> + if ( out_fifo_level == 0 )
> + vpl011->uartfr |= TXFE;
> +
> VPL011_UNLOCK(d, flags);
> }
>
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index 1b583dac3c..db95ff822f 100644
> --- a/xen/include/asm-arm/vpl011.h
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -28,6 +28,8 @@
> #define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
> #define VPL011_UNLOCK(d,flags)
> spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
>
> +#define SBSA_UART_FIFO_SIZE 32
> +
> struct vpl011 {
> void *ring_buf;
> struct page_info *ring_page;
> --
> 2.14.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |