Re: [Xen-devel] [PATCH 27/27 v10] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Dave Martin <Dave.Martin@xxxxxxx>]

On Fri, Sep 22, 2017 at 01:53:26PM +0530, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large output is
> generated such as on running 'find /', output was getting truncated 
> intermittently
> due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that when
> a TX interrupt is asserted then the FIFO queue should be atleast half empty 
> and
> that it can write N bytes in the FIFO, where N is half the FIFO queue size, 
> without
> the bytes getting dropped due to FIFO getting full.
> 
> The SBSA UART emulation logic was asserting the TX interrupt as soon as
> any space became available in the FIFO and the SBSA UART driver tried to write
> more data (upto 16 bytes) in the FIFO expecting that there is enough space
> available leading to dropped bytes.
> 
> The SBSA spec [1] does not specify when the TX interrupt should be asserted
> or de-asserted. Due to lack of clarity on the expected behavior, it is
> assumed for now that TX interrupt should be asserted only when the FIFO goes
> half empty.
> 
> TBD: Once the SBSA spec is updated with the expected behavior, the 
> implementation
> will be modified to align with the spec requirement.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Andre Przywara <andre.przywara@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>

(Taking a quick look at this because I remember fighthing with FIFO
behaviour issues when hacking the Linux driver -- but beware, I'm not a
Xen guy...)


Should this patch be flattened into the patches is fixes?  Keeping
known-wrong code in the series does not help reviewers (but maybe it's
the Xen way).

> Changes since v8:
> - Used variables fifo_level/fifo_threshold for more clarity
> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number

What's sizeof(intf->in), sizeof(intf->out)?

For correct operation, you assume that the total ring buffer size is >=
SBSA_UART_FIFO_SIZE, but nothing enforces this.  If the xencons ring
buffer size is set elsewhere and can't be chosen by a driver, you may at
least add a build-time assert to check that it's big enough.

[...]

> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, uint8_t 
> data)

[...]

> +         * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which
> +         * is the trigger level for asserting/de-assterting the TX interrupt.
>           */
> -        vpl011->uartfr |= BUSY;
> +        fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2;
> +
> +        if ( fifo_level <= fifo_threshold )
> +            vpl011->uartris |= TXI;
> +        else
> +            vpl011->uartris &= ~TXI;
>      }
> +    else
> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
>      vpl011->uartfr &= ~TXFE;
>  

[...]

> @@ -353,37 +370,51 @@ 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 )
> +    if ( in_fifo_level != 0 )
>      {
>          vpl011->uartfr &= ~RXFE;
> -        if ( in_ring_qsize == sizeof(intf->in) )
> +
> +        if ( in_fifo_level == sizeof(intf->in) )
>              vpl011->uartfr |= RXFF;
> +
>          vpl011->uartris |= RXI;
>      }
>  
>      /* Update the uart tx state if the buffer is not full. */
> -    if ( out_ring_qsize != sizeof(intf->out) )
> +    if ( out_fifo_level != sizeof(intf->out) )
>      {
> +        unsigned int out_fifo_threshold;
> +
>          vpl011->uartfr &= ~TXFF;
> -        vpl011->uartris |= TXI;
>  
>          /*
> -         * Clear the BUSY bit as soon as space becomes available
> +         * Clear the BUSY bit as soon as space becomes avaliable
>           * so that the SBSA UART driver can start writing more data
>           * without any further delay.
>           */
>          vpl011->uartfr &= ~BUSY;
>  
> -        if ( out_ring_qsize == 0 )
> +        /*
> +         * 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.
> +         */
> +        out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
> +
> +        if ( out_fifo_level <= out_fifo_threshold )
> +            vpl011->uartris |= TXI;
> +        else
> +            vpl011->uartris &= ~TXI;

Should this logic be factored out?  You do the same thing in
_write_data().

Also, is there a reason why you implement the trigger threshold logic on
the TX side only?  It looks inconsistent now.

I think a real PL011 implements the trigger logic in exactly the same
way for RX and TX (except for swapping >= for <= in the threshold
comparison).


It doesn't look like the Linux pl011 driver relies on a correctly
implemented RX trigger level today, but it may have done in the past --
I did some hacking in this area at some point, but can't remember the
details now.

Asserting RXI whenever the RX FIFO is nonempty would result in excessive
interrupts if you are streaming the data from a slow source (such as a
real UART) and pushing the chars one by one to the emulated UART: the
guest would take an IRQ on each char rather than waiting until the RX
FIFO is half-full.


Cheers
---Dave


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel