[Qemu-devel] Re: [PATCH] qemu/virtio-net: remove wrong s/g layout assumptions

[Michael S. Tsirkin]

On Wed, Nov 25, 2009 at 12:20:05AM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 24, 2009 at 03:57:48PM -0600, Anthony Liguori wrote:
> > Michael S. Tsirkin wrote:
> >> It's useful because this way I won't have to maintain the fix, and it
> >> will make it possible for guests to experiment with layouts, without
> >> hacking qemu. If someone wants to make it a product, that's a different
> >> thing.
> >>   
> >
> > Advertising a new feature is not hard.  It's one line of code in qemu  
> > with Rusty's ACK.
> 
> Okay. Rusty, ACK?
> 
> >> Also, it might be a valid thing for a guest to say that host needs to be
> >> fixed. Not everyone might care about running on any possible broken qemu
> >> version.
> >>
> >> Finally - where do we draw the line? Does any bugfix need a feature bit?
> >>   
> >
> > This is a spec bug, not a qemu bug IMHO.
> >
> > The kernel drivers have always behaved this way and when this was all  
> > written originally, the semantics were never defined.  All the userspace  
> > implementations relied on this.  The fact that the spec claims a  
> > different behavior is a result of deciding that it should be different  
> > after the fact.
> >
> > Thinking about it more, your patch is broken.  If you run it against a  
> > really old guest, it will break badly.
> 
> From what I can tell, my patch will behave exactly as
> existing code does unless guest puts more data than virtio net hdr
> size in the first element. In that last case, qemu called exit(1)
> and I handle it properly.
> 
> > You assume that the guest header is always a fixed size.  It's not,  
> > we've added fields as we've added feature bits.
> 
> Hmm. How so? Look at old qemu code I am replacing. qemu
> just exits if header size is different from sizeof virtio_net_hdr.
> 
> > You actually have to  
> > look at the set of Ack'd features bits to determine how large the header 
> > is.
> 
> I think you are speaking about the mergeable header thing.
> If you look at the code, I think you will see that I handle this
> case correctly.
> 
> > Which is why I now remember why this has never changed.  It's a PITA :-)  
> > I'm not even sure you can do it in a correct way.
> 
> I think I got it right. If not, let me know which feature is handled
> wrong please.

Anthony could you comment please?  I think the patch is correct, and I
even tested on a relatively old guest.
If you still think it is wrong, please tell me why.

> > -- 
> > Regards,
> >
> > Anthony Liguori