Re: [Xen-devel] [PATCH v3 2/2] arm/xen: vpl011: Fix SBSA UART interrupt assertion

[Julien Grall]

Hi Stefano,

On 26/10/17 00:50, Stefano Stabellini wrote:

On Tue, 24 Oct 2017, Andre Przywara wrote:

From: Bhupinder Thakur 

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 
Reviewed-by: Andre Przywara 
Signed-off-by: Andre Przywara 


This is good, only minor cosmetic comments.

Acked-by: Stefano Stabellini 


Julien, can we have the release-ack?


Sure. For the 2 patches:

Release-acked-by: Julien Grall 

Cheers,






---
  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 
  #include 
  #include 
@@ -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 <= 

Re: [Xen-devel] [PATCH v3 2/2] arm/xen: vpl011: Fix SBSA UART interrupt assertion

[Stefano Stabellini]

On Tue, 24 Oct 2017, Andre Przywara wrote:
> From: Bhupinder Thakur 
> 
> 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 
> Reviewed-by: Andre Przywara 
> Signed-off-by: Andre Przywara 

This is good, only minor cosmetic comments.

Acked-by: Stefano Stabellini 


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 
>  #include 
>  #include 
> @@ -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.
> + 

[Xen-devel] [PATCH v3 2/2] arm/xen: vpl011: Fix SBSA UART interrupt assertion

[Andre Przywara]

From: Bhupinder Thakur 

With the current SBSA UART emulation, streaming larger amounts of data
(as caused by "find /", for instance) can lead to character loses.
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 
Reviewed-by: Andre Przywara 
Signed-off-by: Andre Przywara 
---
 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 
 #include 
 #include 
@@ -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));
+
+/* 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 =