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

[Edgar E. Iglesias]

On Tue, Sep 21, 2010 at 11:17:07AM +0200, Michael S. Tsirkin wrote:
> 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
> > > >> <address@hidden>  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 <address@hidden>
> > > 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 <address@hidden>
> > > 
> > > 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.

And at the moment e1000 also has padding, both padding
and FCS appending should go away from ethernet models before
this goes in.

Anyway, if you guys maintaining the networking parts are in
agreement that padding and FCS appending should be done in
the device models (at least for the moment), I'll accept
that and back-off. In that case, I think your suggestion
of hiding things behind some kind of generic macro or
function would be good. At least it will clarify things.

Cheers