On Mon, 2013-02-04 at 14:22 +0200, Michael S. Tsirkin wrote: > 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 <m...@tls.msk.ru> 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."
With all my respect to the virtio spec, let me point that Windows lives by different rules: http://msdn.microsoft.com/en-us/library/windows/hardware/gg463287.aspx > 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 > > > > > >