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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

On 11 October 2017 at 15:38, Dave Martin <Dave.Martin@xxxxxxx> wrote:
> On Wed, Oct 11, 2017 at 01:28:44PM +0530, Bhupinder Thakur wrote:
>> Hi Dave,
>>
>> On 26 September 2017 at 20:08, Dave Martin <Dave.Martin@xxxxxxx> wrote:
>> > 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.
>> >
>> I will add an assert to check this condition.
>>
>> > [...]
>> >
>> >> @@ -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().
>> I will add a common function to set the TXI bit.
>> >
>> > Also, is there a reason why you implement the trigger threshold logic on
>> > the TX side only?  It looks inconsistent now.
>> I did try with RX FIFO threshold also but it seems the current pl011
>> driver does not
>> poll on the RX FIFO and just waits for the RX interrupt trigger to
>> start processing the RX data.
>> This makes RX very slow and if the RX FIFO is not filled sufficiently,
>> it does not read data further.
>> >
>> > 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.
>> >
>> The current pl011 driver
>> > 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.
>> >
>> I agree it is an overhead. This may be an issue with the driver which
>> is solely depending on the RX
>> interrupt. I think it should switch to polling if there are no
>> interrupts received recently.
>
> Hmmm, good point, but isn't that what the receive timeout interrupt is
> supposed to be for?
>
> The Linux driver seems to rely on the receive timeout interrupt
> to recover from an RX stall when the FIFO is not empty but also not full
> enough to trigger the RX FIFO interrupt.
>
> Does your driver actually implement the receive timeout interrupt?
> I'm not very familiar with the code, so I may have missed it.

This patch emulates the SBSA UART spec, which is a subset of the pl011
UART. The SBSA spec [1], Appendix B does not define the requirement of
supporting RX timeout interrupt.

[1] 
https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf

Regards,
Bhupinder

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