Re: [PATCH 2/2] hw/net/allwinner-sun8i-emac: traverse transmit queue using TX_CUR_DESC register value

[Philippe Mathieu-Daudé]

On 2/11/21 11:00 PM, Niek Linnenbank wrote:
> Currently the emulated EMAC for sun8i always traverses the transmit queue
> from the head when transferring packets. It searches for a list of consecutive
> descriptors whichs are flagged as ready for processing and transmits their 
> payloads
> accordingly. The controller stops processing once it finds a descriptor that 
> is not
> marked ready.
> 
> While the above behaviour works in most situations, it is not the same as the 
> actual
> EMAC in hardware. Actual hardware uses the TX_CUR_DESC register value to keep 
> track
> of the last position in the transmit queue and continues processing from that 
> position
> when software triggers the start of DMA processing. The currently emulated 
> behaviour can
> lead to packet loss on transmit when software fills the transmit queue with 
> ready
> descriptors that overlap the tail of the circular list.
> 
> This commit modifies the emulated EMAC for sun8i such that it processes
> the transmit queue using the TX_CUR_DESC register in the same way as hardware.
> 
> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> ---
>  hw/net/allwinner-sun8i-emac.c | 58 +++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 26 deletions(-)

LGTM but I'd feel safer with another review on top.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
> index 042768922c..e586c147e5 100644
> --- a/hw/net/allwinner-sun8i-emac.c
> +++ b/hw/net/allwinner-sun8i-emac.c
> @@ -339,35 +339,40 @@ static void 
> allwinner_sun8i_emac_update_irq(AwSun8iEmacState *s)
>      qemu_set_irq(s->irq, (s->int_sta & s->int_en) != 0);
>  }
>  
> -static uint32_t allwinner_sun8i_emac_next_desc(AwSun8iEmacState *s,
> -                                               FrameDescriptor *desc,
> -                                               size_t min_size)
> +static bool allwinner_sun8i_emac_desc_owned(FrameDescriptor *desc,
> +                                            size_t min_buf_size)
>  {
> -    uint32_t paddr = desc->next;
> +    return (desc->status & DESC_STATUS_CTL) && (min_buf_size == 0 ||
> +           (desc->status2 & DESC_STATUS2_BUF_SIZE_MASK) >= min_buf_size);
> +}
>  
> -    dma_memory_read(&s->dma_as, paddr, desc, sizeof(*desc));
> +static void allwinner_sun8i_emac_get_desc(AwSun8iEmacState *s,
> +                                          FrameDescriptor *desc,
> +                                          uint32_t phys_addr)
> +{
> +    dma_memory_read(&s->dma_as, phys_addr, desc, sizeof(*desc));
> +}
>  
> -    if ((desc->status & DESC_STATUS_CTL) &&
> -        (desc->status2 & DESC_STATUS2_BUF_SIZE_MASK) >= min_size) {
> -        return paddr;
> -    } else {
> -        return 0;
> -    }
> +static uint32_t allwinner_sun8i_emac_next_desc(AwSun8iEmacState *s,
> +                                               FrameDescriptor *desc)
> +{
> +    const uint32_t nxt = desc->next;
> +    allwinner_sun8i_emac_get_desc(s, desc, nxt);
> +    return nxt;
>  }
>  
> -static uint32_t allwinner_sun8i_emac_get_desc(AwSun8iEmacState *s,
> -                                              FrameDescriptor *desc,
> -                                              uint32_t start_addr,
> -                                              size_t min_size)
> +static uint32_t allwinner_sun8i_emac_find_desc(AwSun8iEmacState *s,
> +                                               FrameDescriptor *desc,
> +                                               uint32_t start_addr,
> +                                               size_t min_size)
>  {
>      uint32_t desc_addr = start_addr;
>  
>      /* Note that the list is a cycle. Last entry points back to the head. */
>      while (desc_addr != 0) {
> -        dma_memory_read(&s->dma_as, desc_addr, desc, sizeof(*desc));
> +        allwinner_sun8i_emac_get_desc(s, desc, desc_addr);
>  
> -        if ((desc->status & DESC_STATUS_CTL) &&
> -            (desc->status2 & DESC_STATUS2_BUF_SIZE_MASK) >= min_size) {
> +        if (allwinner_sun8i_emac_desc_owned(desc, min_size)) {
>              return desc_addr;
>          } else if (desc->next == start_addr) {
>              break;
> @@ -383,14 +388,14 @@ static uint32_t 
> allwinner_sun8i_emac_rx_desc(AwSun8iEmacState *s,
>                                               FrameDescriptor *desc,
>                                               size_t min_size)
>  {
> -    return allwinner_sun8i_emac_get_desc(s, desc, s->rx_desc_curr, min_size);
> +    return allwinner_sun8i_emac_find_desc(s, desc, s->rx_desc_curr, 
> min_size);
>  }
>  
>  static uint32_t allwinner_sun8i_emac_tx_desc(AwSun8iEmacState *s,
> -                                             FrameDescriptor *desc,
> -                                             size_t min_size)
> +                                             FrameDescriptor *desc)
>  {
> -    return allwinner_sun8i_emac_get_desc(s, desc, s->tx_desc_head, min_size);
> +    allwinner_sun8i_emac_get_desc(s, desc, s->tx_desc_curr);
> +    return s->tx_desc_curr;
>  }
>  
>  static void allwinner_sun8i_emac_flush_desc(AwSun8iEmacState *s,
> @@ -470,7 +475,8 @@ static ssize_t 
> allwinner_sun8i_emac_receive(NetClientState *nc,
>          bytes_left -= desc_bytes;
>  
>          /* Move to the next descriptor */
> -        s->rx_desc_curr = allwinner_sun8i_emac_next_desc(s, &desc, 64);
> +        s->rx_desc_curr = allwinner_sun8i_emac_find_desc(s, &desc, desc.next,
> +                                                         
> AW_SUN8I_EMAC_MIN_PKT_SZ);
>          if (!s->rx_desc_curr) {
>              /* Not enough buffer space available */
>              s->int_sta |= INT_STA_RX_BUF_UA;
> @@ -495,10 +501,10 @@ static void 
> allwinner_sun8i_emac_transmit(AwSun8iEmacState *s)
>      size_t transmitted = 0;
>      static uint8_t packet_buf[2048];
>  
> -    s->tx_desc_curr = allwinner_sun8i_emac_tx_desc(s, &desc, 0);
> +    s->tx_desc_curr = allwinner_sun8i_emac_tx_desc(s, &desc);
>  
>      /* Read all transmit descriptors */
> -    while (s->tx_desc_curr != 0) {
> +    while (allwinner_sun8i_emac_desc_owned(&desc, 0)) {
>  
>          /* Read from physical memory into packet buffer */
>          bytes = desc.status2 & DESC_STATUS2_BUF_SIZE_MASK;
> @@ -524,7 +530,7 @@ static void 
> allwinner_sun8i_emac_transmit(AwSun8iEmacState *s)
>              packet_bytes = 0;
>              transmitted++;
>          }
> -        s->tx_desc_curr = allwinner_sun8i_emac_next_desc(s, &desc, 0);
> +        s->tx_desc_curr = allwinner_sun8i_emac_next_desc(s, &desc);
>      }
>  
>      /* Raise transmit completed interrupt */
>