Re: [virtio] [OASIS Issue Tracker] Created: (VIRTIO-35) race condition with multi-dword config accesses

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

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, Oct 09, 2013 at 03:33:33PM +0100, Pawel Moll wrote:
>> On Tue, 2013-10-08 at 21:59 +0100, Michael S. Tsirkin wrote:
>> > > > MUST always look at the
>> > > > VIRTIO_1 feature bit to check whether device supports the new interface
>> > > > (as opposed to checking the revision).
>> > > 
>> > > The version register has one intention - make the changes of the control
>> > > registers layout (except for the first two of them :-) easy. Sounds like
>> > > this is where we are. No if, buts or guessing - different register
>> > > layout, different version. So that's the transport.
>> > > 
>> > > The feature bit is still required for the functional drivers (block,
>> > > net, scsi etc). Unless I don't understand the idea :-)
>> > > 
>> > > It seems that it would be possible to have a legacy functional device
>> > > with new MMIO control register layout. I'm not sure this would be
>> > > useful, but it should be possible.
>> > 
>> > I'm sorry I don't understand. We have a set of terms
>> > defined in the spec now- transitional non transitinal legacy.
>> > Can we frame discussion in these terms please?
>> > Changing version makes it a non transitional device.
>> > I am asking for ability to make transitinal
>> > devices as well, this means that
>> > - devices must have ability to have version 1
>> > - drivers must not use version to detect legacy interface
>> 
>> You forgot to define what do "device" and "driver" mean then :-P
>> 
>> I look at the problem as being divided into the transport interface
>> (driven in Linux by platform_driver virtio_mmio_driver) and the virtio
>> device (driven by struct virtio_driver virtio_blk). Is there any problem
>> with this point of view?
>> 
>> Now, the virtio_blk driver can be legacy, transitional or
>> non-transitional. I have no problem with that. MMIO transport is
>> completely transparent in terms of features, including
>> VIRTIO_F_VERSION_1.
>> 
>> At the same time virtio_mmio *driver* will be able to drive only version
>> 1 of the transport, only version 2 of the transport or both versions of
>> the transport. The host-side transport implementation will have register
>> layout of version 1 *or* 2 and will *not* change behaviour in runtime
>> depending on VIRTIO_F_VERSION_1. Is there any problem with this?
>> If you
>> prefer I can call version 1 "legacy", and version 2 "non-transitional".
>> I have no problem with this.
>> 
>> I believe that with this approach, everything should Just Work (TM).
>> 
>> Paweł
>
> Fine, but incomplete.
>
> What I am saying is that version field must not be the version of
> the transport.
> It must only be there to declare that device is non transitional.
>
> The virtio_mmio driver must not look at
> the version field to detect whether device supports virtio 1.0.
> Instead it must only look at whether device exposes VIRTIO_F_VERSION_1.

I don't see the distinction.  The version field controls the mmio
config layout, and that changes with spec v1.0.

Now let's consider a hypothetical virtio 2.0, and assume mmio still
doesn't care about transitional devices.

- Such devices would have the VIRTIO_F_VERSION_2 bit set, and not
  VIRTIO_F_VERSION_1.
- If the mmio config layout changes, mmio would expose this as version 3
  otherwise it would stay as version 2.

So would argue that the mmio transport should not check for
VIRTIO_F_VERSION_1; that's the drivers' job.

> I believe with this approach,  everything should Just Work (TM)
> *and* if someone wants to implement a transitional device
> after all, one will be able to .

I agree that transitional devices aren't possible with this scheme.
However, it *is* simple, and that's the point of virtio-mmio, so I
support Pawel on this.

Cheers,
Rusty.
PS.  Non-transitional drivers MUST refuse to load if !VIRTIO_F_VERSION_1, too.