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 Fri, Oct 04, 2013 at 01:06:43PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Tue, Sep 24, 2013 at 08:18:20PM +0930, Rusty Russell wrote:
>> >> I don't think qemu supports changing a disk underneath a device anyway?
>> >
>> > QEMU supports resizing I think.
>> >
>> >> And in practice, I'm not sure it would ever change fast enough to
>> >> trigger this race...
>> >
>> > Well it's easy to make guest very slow :)
>> >
>> >> So, should we try to fix it?
>> >> Rusty.
>> >
>> > Not really sure but I thought I'd put it on the table.
>> 
>> OK, I got ambitious!  How's this look?
>> 
>> Cheers,
>> Rusty.
>> 
>> commit 9aebe9c69be1461dd0110dfc61459a4ac7bda6f9
>> Author: Rusty Russell <rusty@au1.ibm.com>
>> Date:   Fri Oct 4 13:01:35 2013 +0930
>> 
>>     Configuration (read) atomicity.
>>     
>>     Aka issue VIRTIO-35.
>>     
>>     This is solved per transport:
>>     1) PCI: use the 8 bit reserved field.
>>     	Assume that if you really change that fast, you'll do it lazily on
>>     	config space read.
>
> I guess it's possible ... but isn't it easier to just make it a 32 bit field
> than add this lazy update logic to devices?

I was trying to keep it tight.  I think the lazy update logic is
pretty trivial:

        bool config_changed;
        ....


        on_config_generation_read()
        {
                if (config_changed) {
                        gen++;
                        config_changed = false;
                }
                return gen;
        }

>>     2) MMIO: use offset 0x18.
>>     	Qemu returns 0 when they read here anyway, so it's almost 100%
>>     	backwards compatible (legacy drivers should still multi-read
>>     	for any transport).
>>     3) CCW: no transport changes.
>>     	They always read/write the entire thing.  This just shows that
>>     	Cornelia is smarter than I am.
>>     
>>     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
>> 
>> diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
>> index b5a5835..2945db4 100644
>> --- a/virtio-v1.0-wd01-part1-specification.txt
>> +++ b/virtio-v1.0-wd01-part1-specification.txt
>> @@ -201,11 +201,36 @@ Interface' in the section title.
>>  -------------------------
>>  
>>  Configuration space is generally used for rarely-changing or
>> -initialization-time parameters.
>> +initialization-time parameters.  Drivers must not assume reads from
>> +fields greater than 32 bits wide are atomic, nor or reads from
>> +multiple fields.
>> +
>> +Each transport provides a generation count for the configuration
>> +space, which must change whenever there is a possibility that two
>
> MUST?
>
>> +accesses to the configuration space can see different versions of that
>> +space.
>> +
>> +Thus drivers should read configuration space fields like so:
>
> SHOULD?

The convention is to use lower case for OASIS.  It says in the template
preamble.  I guess it's bold in more sophisticated formats.

>> +
>> +	u32 before, after;
>> +	do {
>> +		before = get_config_generation(device);
>> +		// read config entry/entries.
>> +		after = get_config_generation(device);
>> +	} while (after != before);
>
> BTW this implies reads must never have side
> effects (PCI ISR is the only exception, isn't it?).

Yes.  That would be weird...

Cheers,
Rusty.