Re: [Qemu-devel] [PATCH] e1000: Pad short frames to minimum size (60 bytes)

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Sep 20, 2010 at 10:51:36PM +0200, Edgar E. Iglesias wrote:
> On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> > On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> > >    
> > >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> > >> <edgar.iglesias@gmail.com>  wrote:
> > >>      
> > >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> > >>>        
> > >> I agree.  NICs that do padding will do it on transmit, not receive.
> > >> Anything coming in on the wire should already have the minimum length.
> > >>      
> > > QEMU never gets access to the wire.
> > > Our APIs do not really pass complete ethernet packets:
> > > we forward packets without checksum and padding.
> > >
> > > I think it makes complete sense to keep this and
> > > handle padding in devices because we
> > > have devices that pass the frame to guest without padding and checksum.
> > > It should be easy to replace padding code in devices that
> > > need it with some kind of macro.
> > >    
> > 
> > Would this not also address the problem?  It sounds like the root cause 
> > is the tap code, not the devices..
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >    
> > >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> > >> ne2000 already do this same padding.  This patch is the smallest
> > >> change to cover e1000.
> > >>
> > >>      
> > >>> IMO this kind of padding should somehow be done by the bridge that forwards
> > >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> > >>>        
> > >> That should work and we can then drop the padding code from existing
> > >> NICs.  I'll take a look.
> > >>
> > >> Stefan
> > >>      
> > >    
> > 
> 
> > From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> > From: Anthony Liguori <aliguori@us.ibm.com>
> > Date: Mon, 20 Sep 2010 15:29:31 -0500
> > Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> > 
> > This is required by ethernet drivers but not enforced in the Linux tap code so
> > we need to fix it up ourselves.
> > 
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > 
> > diff --git a/net/tap.c b/net/tap.c
> > index 4afb314..822241a 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
> >  #ifndef __sun__
> >  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
> >  {
> > -    return read(tapfd, buf, maxlen);
> > +    ssize_t len;
> > +
> > +    len = read(tapfd, buf, maxlen);
> > +    if (len > 0) {
> > +        len = MAX(MIN(maxlen, 40), len);
> 
> 
> A small detail :)
> 40 -> 64 (including a dummy FCS).

I don't think so: e1000 at least has code to tack the FCS on,
so we'll end up with a 68 bytes.

> > +    }
> > +    return len;
> >  }
> >  #endif
> >  
> > -- 
> > 1.7.0.4
> >