Re: [Qemu-devel] Headsup: windows virtio networking does not work on current git

[Michael S. Tsirkin]

On Mon, Feb 04, 2013 at 02:00:46PM +0200, Yan Vugenfirer wrote:
> 
> On Feb 4, 2013, at 1:30 AM, Vadim Rozenfeld wrote:
> 
> > On Sun, 2013-02-03 at 15:09 -0600, Anthony Liguori wrote:
> >> Michael Tokarev <address@hidden> writes:
> >> 
> >>> 03.02.2013 17:23, Yan Vugenfirer wrote:
> >>> 
> >>>>> If it helps, mq changes the config size from 8 to 16 bytes.  If the
> >>>>> driver was making an assumption about an 8-byte config size, that's
> >>>>> likely what the problem is.
> >>>>> 
> >>>> That's exactly the problem.
> >>> 
> >>> So what do we do?  It isn't nice to break existing setups.
> >>> Maybe mq can be disabled by default (together with Antony's
> >>> patch) until fixed drivers will be more widely available?
> >> 
> >> I've got a patch that does exactly like this.  It's pretty ugly though
> >> because of the relationship between virtio-pci and virtio-net.  I'm
> >> going to try to see if I can find a cleaner way to do it on Monday.
> >> 
> >> I'm also contemplating just disabling mq by default.  Since a special
> >> command line is needed to enable it anyway (on the backend), having to
> >> specify mq=on for the device doesn't seem so bad.
> >> 
> >> But yeah, we don't want Windows guests to break with -M pc by default so
> >> we should do something to work around it.
> >> 
> >> N.B. this is a pretty nasty bug in the guest driver.  Any new virtio-net
> >> feature is going to trigger it (not just multiqueue).  So while pc-1.3
> >> will work forever with this guest image, it's pretty much guaranteed to
> >> break at some point in the future.
> >> 
> >>> It's easy to turn off mq by default and turn it on when
> >>> needed...
> >>> 
> >>> The problem now is that it isn't obvious why your existing
> >>> setup breaks when you upgrade.  While when you especially
> >>> play with mq, you may look at options available around it...
> >> 
> >> If we ever to get virtio2, a really handy feature to have would be a
> >> driver identification string.  Even if was only informative (and not
> > 
> > Why not just use revision id for that?
> 
> That's what would be expected from real HW if size of the BAR is changed.

virtio spec is very explicit that revision never changes:
"The device must also have a Revision ID of 0 to match this
specification."
It also explicitly documents the use of feature bits for
adding new fields:

"In particular, new fields in the device configuration header are
indicated by offering a feature bit, so the guest can check before
accessing that part of the configuration space.

This allows for forwards and backwards compatibility: if the device is
enhanced with a new feature bit, older guests will not write that
feature bit back to the Guest Features field and it can go into
backwards compatibility mode. Similarly, if a guest is enhanced with a
feature that the device doesn't support, it will not see that feature
bit in the Device Features field and can go into backwards compatibility
mode (or, for poor implementations, set the FAILED Device Status bit).
"

I really don't see how this can be interpreted except as a
promise to add fields and a requirement for guest drivers
to be forward compatible.

> > It really can be very useful, at
> > least for virtio Windows drivers.
> > BTW, Yan already fixed this problem in the Windows driver code. So we
> > can make a new build and make it public. But it probably will not be
> > WHQL'ed in the nearest future.
> > 
> >> authoritative, we've had a lot of issues like this and being able to
> >> make work arounds conditional on the driver identification string would
> >> be nice.
> >> 
> >> Regards,
> >> 
> >> Anthony Liguori
> >> 
> >>> 
> >>> How difficult it is to fix it in win driver?
> >>> 
> >>> Thanks,
> >>> 
> >>> /mjt
> > 
> >