Re: [PATCH RESEND] hw/net: fsl_etsec: Do not reject short frames

[Peter Maydell]

On Mon, 8 Feb 2021 at 14:53, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> As of today both slirp and tap networking do not pad short frames
> (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
>
> If eTSEC is programmed to reject short frames, ARP requests will be
> dropped, preventing the guest from becoming visible on the network.
>
> The same issue was reported on e1000 and vmxenet3 before, see:
>
> commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")

How a short frame should be handled is ethernet device specific:
what is correct for one device model doesn't necessarily apply
to another.

> Ideally this should be fixed on the slirp/tap networking side to
> pad short frames to the minimum frame length, but I am not sure
> whether that's doable.

It would be useful to investigate further exactly where these
short frames are coming from. If one guest is sending out short
frames, or we are doing tap networking and get a genuine short
frame from some external host then we should pass them to the
guest as short frames; if QEMU itself is generating frames (eg
from the 'fake' hosts in usermode networking) then it should be
generating valid frames, not bogus ones, and we should fix whatever
bit of code that is.

> This commit changes to codes to ignore the RCTRL_RSF setting and
> still allow receiving the short frame. The log message is updated
> to mention the reject short frames functionality is unimplemented.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
> RESEND using correct email address
>
>  hw/net/fsl_etsec/rings.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 121415a..503b4d3 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t 
> *buf, size_t size)
>          return -1;
>      }
>
> +    /*
> +     * Both slirp and tap networking do not pad short frames
> +     * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> +     *
> +     * If eTSEC is programmed to reject short frames, ARP requests
> +     * will be dropped, preventing the guest from becoming visible
> +     * on the network.
> +     */
>      if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
>          /* CRC is not in the packet yet, so short frame is below 60 bytes */
> -        RING_DEBUG("%s: Drop short frame\n", __func__);
> -        return -1;
> +        RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
>      }

This doesn't look right. If the guest programs the device to
reject frames less than 60 bytes and then expects to recieve a
frame that's less than 60 bytes, that's a guest bug. If QEMU
itself is generating packets to send and they're short that sounds
like a bug elsewhere in QEMU.

But I think the actual problem here is much simpler:
the datasheet says
# RSF: Receive short frame mode. When set, enables the reception of
# frames shorter than 64 bytes. [...]
#    0 Ethernet frames less than 64B in length are silently dropped
#    1 Frames less than 64B are accepted upon a DA match
(https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf chapter 14)

whereas the QEMU code is doing the reverse: dropping short
packets if the bit is 1.

If you fix this bug by reversing the sense of the test on the
RSF bit, does it make your guest happier ?

thanks
-- PMM