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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

On 18 October 2017 at 15:56, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Hi,
>
> On 13/10/17 11:40, 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.
>
> So similar to the other patch:
>
> - I can confirm that this patch fixes the dropped characters issue we
> see with current staging HEAD. And, differently from the first patch,
> this one fixes a correctness issue (we are loosing characters at the
> moment) rather than just a performance problem. So I think we definitely
> need something along those lines.
>
> However ... ;-)
> Asserting the receive interrupt at the first character, while it is
> architected to be only triggered at half the FIFO level, is not right.
> Instead what we probably want it to use the timeout interrupt instead. I
> quickly hacked something up like that:
> - In vpl011_data_avail() we assert the timeout interrupt (RTI) if the
> in-FIFO is not empty. This is following the idea that when this function
> is called, Xen says: this is all the data I have at the moment. The
> guest should be able to see the data, because Xen has no idea when and
> if more data will come in.
> - If we drain the in-FIFO in vpl011_mmio_read() (fifo_level becomes 0),
> we clear RTI.
> - We handle RXI like described in the spec: assert it in data_avail() if
> the FIFO has 16 or less characters left, clear it in mmio_read() if the
> FIFO has space for more than 16 characters.
I think you meant - RXI should be asserted when FIFO has 16 or more
characters left.
>
> This basically moves the trick of asserting RXI to asserting RTI
> instead, which sounds architecturally cleaner.
>
> Let me try to clean up my approach and post it.
>
> Cheers,
> Andre.
>
>
>
>>
>> [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>
>> CC: Dave Martin <dave.martin@xxxxxxx>
>>
>> Changes since v11:
>> - Add a build assert to check that ring buffer size is more than minimum rx 
>> fif size of 32
>> - Added a comment to explain why threshold based logic is not implemented 
>> for rx fifo
>> - Moved calls to vpl011_update_interrupt_status() near where TXI/RXI status 
>> bit is set
>>
>> 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
>> - Renamed ring_qsize variables to fifo_level for consistency
>>
>>  xen/arch/arm/vpl011.c        | 113 
>> ++++++++++++++++++++++++++++++-------------
>>  xen/include/asm-arm/vpl011.h |   2 +
>>  2 files changed, 82 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index 0b07436..adf1711 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -93,24 +93,27 @@ 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));
>> +
>> +        if ( fifo_level == 0 )
>> +        {
>> +            vpl011->uartfr |= RXFE;
>> +            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;
>> -    }
>> -
>>      vpl011->uartfr &= ~RXFF;
>>
>> -    vpl011_update_interrupt_status(d);
>> -
>>      VPL011_UNLOCK(d, flags);
>>
>>      /*
>> @@ -122,6 +125,26 @@ 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;
>> +
>> +    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.
>> +     */
>> +    fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
>> +
>> +    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 +169,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));
>>
>> -        /*
>> -         * 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) )
>> +        {
>> +            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 +371,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 +382,46 @@ 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;
>> +
>> +        /*
>> +         * Currently, the RXI bit is getting set even if there is a single
>> +         * byte of data in the rx fifo. Ideally, the RXI bit should be set
>> +         * only if the rx fifo level reaches the threshold.
>> +         *
>> +         * However, since currently RX timeout interrupt is not
>> +         * implemented as there is not enough clarity in the SBSA spec,
>> +         * the guest may keep waiting for an interrupt to read more
>> +         * data. To ensure that guest reads all the data without
>> +         * any delay, the RXI interrupt is raised if there is RX data
>> +         * available without checking whether fifo level has reached
>> +         * the threshold.
>> +         *
>> +         * TBD: Once there is more clarity in the SBSA spec on whether RX
>> +         * timeout interrupt needs to be implemented, the RXI interrupt
>> +         * will be raised only when rx fifo level reaches the threshold.
>> +         */
>>          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) )
>>      {
>>          vpl011->uartfr &= ~TXFF;
>> -        vpl011->uartris |= TXI;
>>
>>          /*
>>           * Clear the BUSY bit as soon as space becomes available
>> @@ -385,7 +430,9 @@ static void vpl011_data_avail(struct domain *d)
>>           */
>>          vpl011->uartfr &= ~BUSY;
>>
>> -        if ( out_ring_qsize == 0 )
>> +        vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
>> +
>> +        if ( out_fifo_level == 0 )
>>              vpl011->uartfr |= TXFE;
>>      }
>>
>> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
>> index 1b583da..db95ff8 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;
>>

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