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 <nieklinnenb...@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é <f4...@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 */
>
