Re: [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic

[David Gibson]

On Tue, Feb 09, 2021 at 09:48:18AM +0000, Peter Maydell wrote:
> On Tue, 9 Feb 2021 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Per MPC8548ERM [1] chapter 14.5.3.4.1:
> >
> > When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
> > a DA match. But currently QEMU does the opposite.
> >
> > When RCTRL.RSF is 0, short frames are silently dropped, however
> > we cannot drop such frames in QEMU as of today, due to 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)")
> >
> > 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.
> >
> > This commit reverses the RCTRL.RSF testing logic to match the spec.
> > The log message is updated to mention the reject short frames
> > functionality is unimplemented.
> >
> > [1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
> >
> > Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> 
> > -    if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > +    /*
> > +     * 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 change is doing two things at once.

Oops, I missed that.

> One of them is an entirely uncontroversial bug fix: we
> got the sense of the RCTRL_RSF test the wrong way round.
> 
> The other is different: it is working around a bug elsewhere in QEMU.
> 
> If there's a problem with packets that should not be short
> frames being presented to ethernet devices as short frames,
> please fix that bug at the source. I don't think we should
> take any more device-model workarounds for it. We have lots
> and lots of ethernet device models: it will be much more
> effort to try to fix them all one by one as people encounter
> this bug than it would be to just fix the code that's creating
> bogus short frames.
> 
> David, could you drop this from your queue, please ?

Done.

> 
> thanks
> -- PMM
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature