Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Jason Wang]

On 07/15/2015 02:01 PM, Fam Zheng wrote:
> On Wed, 07/15 13:10, Jason Wang wrote:
>>>> And can we do this without a bh? Otherwise, we may need to stop and
>>>> restart the bh during vm stop and start?
>>> A bh doesn't hurt when vm stop and restart (we get superfluous flush),
>> The problem is qemu_flush_queued_packets() does not check runstate. So
>> it may call .receive() which may modify guest state (DMA or registers).
> You're right, .can_receive will be called incorrectly if the following 
> sequence
> of events is processed by main loop right after we schedule it:
>
>  1) QMP 'stop' command:
>     Runstate is changed to STOP.
>
>  2) tap read:
>     A new packet is read in, but since qemu_can_send_packet is false, it will
>     be queued.
>
>  3) aio_dispatch:
>     This BH is called too late here, and the queue is flushed, which calls
>     .receive().
>
> An ideal fix would be stopping tap with a vmstate handler, but for this patch,
> does the following work?

Looks good for me. How about axienet then since in your series it also
uses a bh?

>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index f5170ae..0f5cf44 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -342,13 +342,22 @@ static ssize_t etsec_receive(NetClientState *nc,
>                               const uint8_t  *buf,
>                               size_t          size)
>  {
> +    ssize_t ret;
>      eTSEC *etsec = qemu_get_nic_opaque(nc);
>  
>  #if defined(HEX_DUMP)
>      fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size);
>      qemu_hexdump(buf, stderr, "", size);
>  #endif
> -    return etsec_rx_ring_write(etsec, buf, size);
> +    /* Flush is unnecessary as are already in receiving path */
> +    etsec->need_flush = false;
> +    ret = etsec_rx_ring_write(etsec, buf, size);
> +    if (ret == 0) {
> +        /* The packet will be queued, let's flush it when buffer is avilable
> +         * again. */
> +        etsec->need_flush = true;
> +    }
> +    return ret;
>  }
>  
>  
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index fc41773..e7dc0a4 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -144,6 +144,8 @@ typedef struct eTSEC {
>      QEMUBH *bh;
>      struct ptimer_state *ptimer;
>  
> +    /* Whether we should flush the rx queue when buffer becomes available. */
> +    bool need_flush;
>  } eTSEC;
>  
>  #define TYPE_ETSEC_COMMON "eTSEC"
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index a11280b..68e7b6d 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -646,6 +646,9 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
>      } else {
>          etsec->rx_buffer_len = 0;
>          etsec->rx_buffer     = NULL;
> +        if (etsec->need_flush) {
> +            qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
> +        }
>      }
>  
>      RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", 
> remaining_data);
>
>