Re: [virtio-dev] [PATCH] transitional issues: add new IDs for all devices

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Sep 25, 2013 at 01:50:04PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Tue, Sep 24, 2013 at 09:10:09PM +0930, Rusty Russell wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> > non-transitional devices should have been able to simply update
>> >> > revision ID to make sure legacy drivers are not loaded.
>> >> > Unfortunately, mistakes were made:
>> >> > - we didn't stress that drivers must check revision ID,
>> >> >   and of course there's no easy way for drivers to
>> >> >   test this failure path,
>> >> >   so older versions of Windows drivers ignored revision
>> >> >   (latest vision matches revision correctly)
>> >> > - CCW lacks revision ID field
>> >> >
>> >> > Both facts mean a non-transitional device would need
>> >> > a separate mechanism to prevent legacy drivers from
>> >> > loading.
>> >> > We aren't running out of device IDs yet, so
>> >> > let's use up some to resolve this.
>> >> >
>> >> > I incremented all IDs by 0x100 intentionally -
>> >> > for the PCI bindings, this should help remind people they can't
>> >> > just stick the Subsystem ID into the low byte of the Device ID.
>> >> 
>> >> I don't think this actually works.
>> >
>> > Hmm what doesn't work, exactly?
>> >
>> >> If you offer a non-transitional device to a legacy driver, it will
>> >> fail anyway (device doesn't work due to missing feature 32).
>> >
>> > How does it fail? Legacy drivers don't give devices any chance
>> > to fail gracefully.
>> 
>> >
>> >> There's not much difference between that and it not finding the device.
>> >
>> > Imagine a non transitional device.
>> > It would want to use offset 0 in BAR0 for something else not config.
>> > When legacy driver attempts to access it, it can cause
>> > all kind of mischief.
>> 
>> As per call, I think we should check that the existing Windows drivers
>> fail if BAR0 does not exist, and then make that a recommendation for
>> PCI, eg:
>> 
>> On a platform where legacy devices can exist, devices should not offer
>> BAR0 at all.
>> 
>> Cheers,
>> Rusty.
>
> Heh no luck at all. My assumption is we'll want to have IO BARs
> somewhere. If that happens drivers do all kind of weird stuff.

Yeah, yuck!

I grabbed the drivers to dig further.  It looks like (if we present an
IO BAR) the first thing it will do is reset the device by writing a 0
byte to offset 18.

If you put virtio_pci_common_cfg at the start of the BAR, that
corresponds to a write to the read-only num_queues entry.  If it's the
ISR BAR it's off the end, and if it's the notification BAR it may be
valid if there are 9 virtqueues, but should never be accessed with a
single byte write.

How about a PCI note something like:

        If a device does not support legacy mode, and is on a platform
        where a legacy device with the same ID has previously existed,
        it MUST take the following steps to fail gracefully when a
        legacy driver attempts to drive it:

        1) Present no I/O BAR, and no BAR 0, OR
        2) Respond to a single-byte zero write to offset 18 of any I/O
           BAR or BAR0 by presenting zeroes on every BAR and ignoring
           writes.

This means it presents 0 virtqueues; at the very least, it makes the
device harmless.

Note that I didn't specify that it should only be the first write: think
of the kexec case, where an OS crash causes an older OS to come in and
try to reset the device...

Would that work?
Rusty.