OASIS Virtual I/O Device (VIRTIO) TC

race condition with multi-dword config accesses ----------------------------------------------- Key: VIRTIO-35 URL: http://tools.oasis-open.org/issues/browse/VIRTIO-35 Project: OASIS Virtual I/O Device (VIRTIO) TC Issue Type: Bug Reporter: Michael Tsirkin on many architectures, accesses larger than 32 bit can not be atomic. Thus access to a device config field of >4 bytes is inherently racy in case field can change. For example, virtio-blk has u64 capacity; The following race can trigger: driver reads low 32 bit both low and high 32 bit change driver reads high 32 bit as a result, capacity observed is composed of old low bits and new high bits which does not make sense. For legacy devices, spec allowed byte by byte access, making the race even more common. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://tools.oasis-open.org/issues/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

OASIS Issues Tracker <workgroup_mailer@lists.oasis-open.org> writes: > race condition with multi-dword config accesses > ----------------------------------------------- > > Key: VIRTIO-35 > URL: http://tools.oasis-open.org/issues/browse/VIRTIO-35 > Project: OASIS Virtual I/O Device (VIRTIO) TC > Issue Type: Bug > Reporter: Michael Tsirkin > > > on many architectures, accesses larger than 32 bit can not be atomic. > Thus access to a device config field of >4 bytes is inherently racy > in case field can change. > > For example, virtio-blk has > u64 capacity; > The following race can trigger: > driver reads low 32 bit > both low and high 32 bit change > driver reads high 32 bit > > as a result, capacity observed is composed of > old low bits and new high bits which does not > make sense. > > For legacy devices, spec allowed byte by byte access, > making the race even more common. Yes, in theory this is a problem with virtio_blk, which could change capacity (the spec is silent on this, but the Linux driver will handle it by re-reading capacity, exposing this race). I don't think qemu supports changing a disk underneath a device anyway? And in practice, I'm not sure it would ever change fast enough to trigger this race... So, should we try to fix it? Rusty.

On Tue, Sep 24, 2013 at 08:18:20PM +0930, Rusty Russell wrote: > OASIS Issues Tracker <workgroup_mailer@lists.oasis-open.org> writes: > > race condition with multi-dword config accesses > > ----------------------------------------------- > > > > Key: VIRTIO-35 > > URL: http://tools.oasis-open.org/issues/browse/VIRTIO-35 > > Project: OASIS Virtual I/O Device (VIRTIO) TC > > Issue Type: Bug > > Reporter: Michael Tsirkin > > > > > > on many architectures, accesses larger than 32 bit can not be atomic. > > Thus access to a device config field of >4 bytes is inherently racy > > in case field can change. > > > > For example, virtio-blk has > > u64 capacity; > > The following race can trigger: > > driver reads low 32 bit > > both low and high 32 bit change > > driver reads high 32 bit > > > > as a result, capacity observed is composed of > > old low bits and new high bits which does not > > make sense. > > > > For legacy devices, spec allowed byte by byte access, > > making the race even more common. > > Yes, in theory this is a problem with virtio_blk, which could change > capacity (the spec is silent on this, but the Linux driver will handle > it by re-reading capacity, exposing this race). > > 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. -- MST

"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. 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 +accesses to the configuration space can see different versions of that +space. + +Thus drivers should read configuration space fields like so: + + u32 before, after; + do { + before = get_config_generation(device); + // read config entry/entries. + after = get_config_generation(device); + } while (after != before); Note that this space is generally the guest's native endian, rather than PCI's little-endian. +2.1.3.1. Legacy Interface: Configuration Space +------------------------- + +Legacy devices did not have a configuration generation field, thus are +susceptible to race conditions if configuration is updated. This +effects the block capacity and network mac fields; best practice is to +read these fields multiple times until two reads generate a consistent +result. + 2.1.4. Virtqueues ---------------- @@ -768,7 +793,7 @@ struct virtio_pci_common_cfg { __le16 msix_config; /* read-write */ __le16 num_queues; /* read-only */ __u8 device_status; /* read-write */ - __u8 unused1; + __u8 config_generation; /* read-only */ /* About a specific virtqueue. */ __le16 queue_select; /* read-write */ @@ -820,6 +845,14 @@ device_status Device Status field. Writing 0 into this field resets the device. +config_generation + + Configuration atomicity value. Changes every time the + configuration noticeably changes. This means the device may + only change the value after a configuration read operation, + but it must change if there is any risk of a device seeing an + inconsistent configuration state. + queue_select Queue Select. Selects which virtqueue do other fields refer to. @@ -941,6 +974,9 @@ Legacy Interface. When used through the Legacy Interface, Transitional Devices must assume that Feature Bits 32 to 63 are not acknowledged by Driver. +As legacy devices had no configuration generation field, see "2.1.3.1. +Legacy Interface: Configuration Space" for workarounds. + 2.3.1.3. PCI-specific Initialization And Device Operation -------------------------------------------------------- @@ -1281,6 +1317,13 @@ configuration space. The following list presents their layout: must write a value to the HostFeaturesSel register before reading from the HostFeatures register. +• 0x018 R ConfigGeneration + Configuration atomicity value. + Changes every time the configuration noticeably changes. This means the + device may only change the value after a configuration read operation, + but it must change if there is any risk of a device seeing an inconsistent + configuration state. + • 0x020 W GuestFeatures Flags representing device features understood and activated by the driver. @@ -1396,6 +1439,12 @@ Writing to registers described as “R” and reading from registers described as “W” is not permitted and can cause undefined behavior. +2.3.2.2.1. Legacy Interface: MMIO Device Layout +-------------------------- +The ConfigGeneration field does not exist in legacy devices; fortunately +it would return 0 if accessed. Nonetheless, the workarounds in +"2.1.3.1. Legacy Interface: Configuration Space" should still be used. + 100.3.2.2.1. Virtqueue Layout ------------------------------ @@ -1631,7 +1680,9 @@ For changing configuration information, the guest may use CCW_CMD_WRITE_CONF, specifying the guest memory for the host to read from. -In both cases, the complete configuration space is transmitted. +In both cases, the complete configuration space is transmitted. This +allows the guest to compare the new configuration space with the old +version, and keep a generation count internally whenever it changes. 2.3.3.2.5. Setting Up Indicators -------------------------------- @@ -3415,12 +3466,15 @@ transmit output. --------------------------------------- Configuration space should only be used for initialization-time -parameters. It is a limited resource with no synchronization, so for -most uses it is better to use a virtqueue to update configuration -information (the network device does this for filtering, +parameters. It is a limited resource with no synchronization between +writable fields, so for most uses it is better to use a virtqueue to update +configuration information (the network device does this for filtering, otherwise the table in the config space could potentially be very large). +Devices must not assume that configuration fields over 32 bits wide +are atomically writable. + 2.7.3. What Device Number? --------------------------

Il 04/10/2013 05:36, Rusty Russell ha scritto: > "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? Looks good. Paolo

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? > 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? > + > + 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?). > > Note that this space is generally the guest's native endian, > rather than PCI's little-endian. > > +2.1.3.1. Legacy Interface: Configuration Space > +------------------------- > + > +Legacy devices did not have a configuration generation field, thus are > +susceptible to race conditions if configuration is updated. This > +effects the block capacity and network mac fields; best practice is to > +read these fields multiple times until two reads generate a consistent > +result. > + > 2.1.4. Virtqueues > ---------------- > > @@ -768,7 +793,7 @@ struct virtio_pci_common_cfg { > __le16 msix_config; /* read-write */ > __le16 num_queues; /* read-only */ > __u8 device_status; /* read-write */ > - __u8 unused1; > + __u8 config_generation; /* read-only */ > > /* About a specific virtqueue. */ > __le16 queue_select; /* read-write */ > @@ -820,6 +845,14 @@ device_status > Device Status field. Writing 0 into this field resets the > device. > > +config_generation > + > + Configuration atomicity value. Changes every time the > + configuration noticeably changes. This means the device may > + only change the value after a configuration read operation, > + but it must change if there is any risk of a device seeing an > + inconsistent configuration state. > + > queue_select > > Queue Select. Selects which virtqueue do other fields refer to. > @@ -941,6 +974,9 @@ Legacy Interface. When used through the Legacy Interface, > Transitional Devices must assume that Feature Bits 32 to 63 > are not acknowledged by Driver. > > +As legacy devices had no configuration generation field, see "2.1.3.1. > +Legacy Interface: Configuration Space" for workarounds. > + > 2.3.1.3. PCI-specific Initialization And Device Operation > -------------------------------------------------------- > > @@ -1281,6 +1317,13 @@ configuration space. The following list presents their layout: > must write a value to the HostFeaturesSel register before > reading from the HostFeatures register. > > +• 0x018 R ConfigGeneration > + Configuration atomicity value. > + Changes every time the configuration noticeably changes. This means the > + device may only change the value after a configuration read operation, > + but it must change if there is any risk of a device seeing an inconsistent > + configuration state. > + > • 0x020 W GuestFeatures > Flags representing device features understood and activated by > the driver. > @@ -1396,6 +1439,12 @@ Writing to registers described as “R” and reading from > registers described as “W” is not permitted and can cause > undefined behavior. > > +2.3.2.2.1. Legacy Interface: MMIO Device Layout > +-------------------------- > +The ConfigGeneration field does not exist in legacy devices; fortunately > +it would return 0 if accessed. Nonetheless, the workarounds in > +"2.1.3.1. Legacy Interface: Configuration Space" should still be used. > + > 100.3.2.2.1. Virtqueue Layout > ------------------------------ > > @@ -1631,7 +1680,9 @@ For changing configuration information, the guest may use > CCW_CMD_WRITE_CONF, specifying the guest memory for the host to > read from. > > -In both cases, the complete configuration space is transmitted. > +In both cases, the complete configuration space is transmitted. This > +allows the guest to compare the new configuration space with the old > +version, and keep a generation count internally whenever it changes. > > 2.3.3.2.5. Setting Up Indicators > -------------------------------- > @@ -3415,12 +3466,15 @@ transmit output. > --------------------------------------- > > Configuration space should only be used for initialization-time > -parameters. It is a limited resource with no synchronization, so for > -most uses it is better to use a virtqueue to update configuration > -information (the network device does this for filtering, > +parameters. It is a limited resource with no synchronization between > +writable fields, so for most uses it is better to use a virtqueue to update > +configuration information (the network device does this for filtering, > otherwise the table in the config space could potentially be very > large). > > +Devices must not assume that configuration fields over 32 bits wide > +are atomically writable. > + > 2.7.3. What Device Number? > -------------------------- >

"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.

On Fri, Oct 11, 2013 at 09:54:12AM +1030, Rusty Russell wrote: > "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. Well we always find some use for the padding space later, or we can move some stuff around to pack it ... > 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; > } And something similar on migration. I agree it's not too bad. But we'll need to document this in the spec somehow. > >> 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.

On Fri, 2013-10-04 at 04:36 +0100, Rusty Russell wrote: > +• 0x018 R ConfigGeneration > + Configuration atomicity value. > + Changes every time the configuration noticeably changes. This means the > + device may only change the value after a configuration read operation, > + but it must change if there is any risk of a device seeing an inconsistent > + configuration state. How about moving it closer to the config space? Something like 0x0f0 or even 0x0fc? > +2.3.2.2.1. Legacy Interface: MMIO Device Layout > +-------------------------- > +The ConfigGeneration field does not exist in legacy devices; fortunately > +it would return 0 if accessed. Not on "my" models, no. It would result in external abort (think SIGBUS). Not a big problem - the device version number will be bumped to 2 anyway (working on it right now). Pawel

On Tue, Oct 08, 2013 at 11:39:10AM +0100, Pawel Moll wrote: > On Fri, 2013-10-04 at 04:36 +0100, Rusty Russell wrote: > > +• 0x018 R ConfigGeneration > > + Configuration atomicity value. > > + Changes every time the configuration noticeably changes. This means the > > + device may only change the value after a configuration read operation, > > + but it must change if there is any risk of a device seeing an inconsistent > > + configuration state. > > How about moving it closer to the config space? Something like 0x0f0 or > even 0x0fc? > > > +2.3.2.2.1. Legacy Interface: MMIO Device Layout > > +-------------------------- > > +The ConfigGeneration field does not exist in legacy devices; fortunately > > +it would return 0 if accessed. > > Not on "my" models, no. It would result in external abort (think > SIGBUS). > > Not a big problem - the device version number will be bumped to 2 anyway > (working on it right now). > > Pawel > Hmm do current guests check version number? Transitional devices should support both old and new guests. For transitional I think the right thing to do is to key off the VIRTIO_1 feature bit and/or FEATURES_OK. -- MST

On Tue, 2013-10-08 at 13:10 +0100, Michael S. Tsirkin wrote: > On Tue, Oct 08, 2013 at 11:39:10AM +0100, Pawel Moll wrote: > > On Fri, 2013-10-04 at 04:36 +0100, Rusty Russell wrote: > > > +• 0x018 R ConfigGeneration > > > + Configuration atomicity value. > > > + Changes every time the configuration noticeably changes. This means the > > > + device may only change the value after a configuration read operation, > > > + but it must change if there is any risk of a device seeing an inconsistent > > > + configuration state. > > > > How about moving it closer to the config space? Something like 0x0f0 or > > even 0x0fc? > > > > > +2.3.2.2.1. Legacy Interface: MMIO Device Layout > > > +-------------------------- > > > +The ConfigGeneration field does not exist in legacy devices; fortunately > > > +it would return 0 if accessed. > > > > Not on "my" models, no. It would result in external abort (think > > SIGBUS). > > > > Not a big problem - the device version number will be bumped to 2 anyway > > (working on it right now). > > > > Pawel > > > > Hmm do current guests check version number? Yes. /* Check device version */ vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION); if (vm_dev->version != 1) { dev_err(&pdev->dev, "Version %ld not supported!
", vm_dev->version); return -ENXIO; } > Transitional devices should support both old and > new guests. We've discussed it before and I have a different opinion. The MMIO devices are meant to be simple and do one thing and one thing only. The guest driver can easily handle both versions. > For transitional I think the right thing to do is > to key off the VIRTIO_1 feature bit and/or FEATURES_OK. One does not preclude another. MMIO devices ver.2 may have this as requirement. Pawel

On Tue, Oct 08, 2013 at 02:06:52PM +0100, Pawel Moll wrote: > On Tue, 2013-10-08 at 13:10 +0100, Michael S. Tsirkin wrote: > > On Tue, Oct 08, 2013 at 11:39:10AM +0100, Pawel Moll wrote: > > > On Fri, 2013-10-04 at 04:36 +0100, Rusty Russell wrote: > > > > +• 0x018 R ConfigGeneration > > > > + Configuration atomicity value. > > > > + Changes every time the configuration noticeably changes. This means the > > > > + device may only change the value after a configuration read operation, > > > > + but it must change if there is any risk of a device seeing an inconsistent > > > > + configuration state. > > > > > > How about moving it closer to the config space? Something like 0x0f0 or > > > even 0x0fc? > > > > > > > +2.3.2.2.1. Legacy Interface: MMIO Device Layout > > > > +-------------------------- > > > > +The ConfigGeneration field does not exist in legacy devices; fortunately > > > > +it would return 0 if accessed. > > > > > > Not on "my" models, no. It would result in external abort (think > > > SIGBUS). > > > > > > Not a big problem - the device version number will be bumped to 2 anyway > > > (working on it right now). > > > > > > Pawel > > > > > > > Hmm do current guests check version number? > > Yes. > > /* Check device version */ > vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION); > if (vm_dev->version != 1) { > dev_err(&pdev->dev, "Version %ld not supported!
", > vm_dev->version); > return -ENXIO; > } > > > Transitional devices should support both old and > > new guests. > > We've discussed it before and I have a different opinion. It's merely a definition (we voted this into spec btw :) ). If you don't support legacy drivers you have a non transitional device. That's fine I guess. > The MMIO > devices are meant to be simple and do one thing and one thing only. The > guest driver can easily handle both versions. To clarify, I'm not asking you to code up a transitional device. I'm merely suggesting that the option is there in the spec. Makes sense? > > For transitional I think the right thing to do is > > to key off the VIRTIO_1 feature bit and/or FEATURES_OK. > > One does not preclude another. MMIO devices ver.2 may have this as > requirement. > > Pawel >

On Tue, 2013-10-08 at 14:15 +0100, Michael S. Tsirkin wrote: > > > Transitional devices should support both old and > > > new guests. > > > > We've discussed it before and I have a different opinion. > > It's merely a definition (we voted this into spec btw :) ). > If you don't support legacy drivers > you have a non transitional device. > That's fine I guess. This is what I meant - I don't mind the "transitional" idea in the spec. I simply don't want to bother MMIO devices authors with maintaining backward compatibility :-) Simply see no value of that in MMIO case. Pawel

On Tue, Oct 08, 2013 at 02:26:35PM +0100, Pawel Moll wrote: > On Tue, 2013-10-08 at 14:15 +0100, Michael S. Tsirkin wrote: > > > > Transitional devices should support both old and > > > > new guests. > > > > > > We've discussed it before and I have a different opinion. > > > > It's merely a definition (we voted this into spec btw :) ). > > If you don't support legacy drivers > > you have a non transitional device. > > That's fine I guess. > > This is what I meant - I don't mind the "transitional" idea in the spec. > I simply don't want to bother MMIO devices authors with maintaining > backward compatibility :-) Simply see no value of that in MMIO case. > > Pawel > Well basically what I am saying is that spec should say that drivers MUST support both revision 1 and revision 2 and MUST always look at the VIRTIO_1 feature bit to check whether device supports the new interface (as opposed to checking the revision). This removes info duplication between revision and feature bit and will allow transitional devices if we ever want them after all, without adding complexity. Is this acceptable to you? -- MST

On Tue, 2013-10-08 at 14:38 +0100, Michael S. Tsirkin wrote: > Well basically what I am saying is that spec should say that drivers > MUST support both revision 1 and revision 2 No, changing the version number just for the sake of changing it makes no sense whatsoever. > 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. > This removes info duplication between revision and feature bit > and will allow transitional devices if we ever want them > after all, without adding complexity. Implementation of the transitional devices *will* be more complex than ones following one *or* the other version of the spec, I think you can't argue that. I sympathize with your effort of making the PCI (server) devices/drivers backward compatible. But also very *very* strong doubts whether the same makes sense in the MMIO world (let's call it "embedded"). I'll make the Linux driver work with both kinds of the devices. Other OSes? Don't care - I'm 99.9% sure that there are no non-Linux MMIO drivers. Running old kernel (legacy driver only) on cutting edge model with new device? Tough, I don't think I care either - upgrade the kernel. > Is this acceptable to you? My acceptance doesn't really matter here - I can be simply voted wrong :-) I'm just trying to maintain the MMIO transport's modus operandi - keep it simple. Anyway, I'll think more about it. Feel free to point out holes in my reasoning ;-) Pawel

On Tue, Oct 08, 2013 at 06:38:12PM +0100, Pawel Moll wrote: > On Tue, 2013-10-08 at 14:38 +0100, Michael S. Tsirkin wrote: > > Well basically what I am saying is that spec should say that drivers > > MUST support both revision 1 and revision 2 > > No, changing the version number just for the sake of changing it makes > no sense whatsoever. It's for the sake of making sure old drivers don't bind to non transitional devices. > > 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 > > This removes info duplication between revision and feature bit > > and will allow transitional devices if we ever want them > > after all, without adding complexity. > > Implementation of the transitional devices *will* be more complex than > ones following one *or* the other version of the spec, I think you can't > argue that. > > I sympathize with your effort of making the PCI (server) devices/drivers > backward compatible. But also very *very* strong doubts whether the same > makes sense in the MMIO world (let's call it "embedded"). I'll make the > Linux driver work with both kinds of the devices. Other OSes? Don't care > - I'm 99.9% sure that there are no non-Linux MMIO drivers. Running old > kernel (legacy driver only) on cutting edge model with new device? > Tough, I don't think I care either - upgrade the kernel. > > > Is this acceptable to you? > > My acceptance doesn't really matter here - I can be simply voted > wrong :-) I'm just trying to maintain the MMIO transport's modus > operandi - keep it simple. > > Anyway, I'll think more about it. Feel free to point out holes in my > reasoning ;-) > > Pawel >

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). Pawel

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). > > Pawel 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 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 . -- MST

On Wed, 2013-10-09 at 17:23 +0100, Michael S. Tsirkin wrote: > 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 completely disagree, but that's not a big problem. Fortunately the spec defines the device behaviour, not the driver implementation. The device compliant with the spec will have to report version 2 - I don't think you're contesting that, are you? Pawel

On Thu, Oct 10, 2013 at 10:59:59AM +0100, Pawel Moll wrote: > On Wed, 2013-10-09 at 17:23 +0100, Michael S. Tsirkin wrote: > > 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 completely disagree, but that's not a big problem. Fortunately the > spec defines the device behaviour, not the driver implementation. It really must do both. If we don't we get bad results: for example we merely stated that device revision is 0 for PCI and did not say that drivers must check it. Result - old windows drivers ignored revision which gets in the way of using that field for non-transitional devices. > The > device compliant with the spec will have to report version 2 - I don't > think you're contesting that, are you? > > Pawel I think I do contest that at the moment. I think a transitional device should report version 1. In legacy drivers version is checked before features are probed so as far as I can see there's no way for device to detect a legacy driver and switch that to 1 dynamically. I'm not saying spec should force anyone to implement transitional devices, but I don't see why it should make this completely impossible. So I think that a device compliant with the spec can report versions 1 or 2. This is why I think drivers must look at features to check support for 1.0. -- MST

On Thu, 2013-10-10 at 12:26 +0100, Michael S. Tsirkin wrote: > I think a transitional device should report version 1. > In legacy drivers version is checked before features are probed > so as far as I can see there's no way for device to detect a legacy driver and > switch that to 1 dynamically. > > I'm not saying spec should force anyone to implement > transitional devices, but I don't see why it should > make this completely impossible. > > So I think that a device compliant with the spec can report versions 1 or 2. I'll repeat myself. The version register defines the registers layout. Different register layout, different version number. And let me put this thing clear - you will not convince my otherwise. You can vote me wrong. Pawel

On Thu, Oct 10, 2013 at 03:53:46PM +0100, Pawel Moll wrote: > On Thu, 2013-10-10 at 12:26 +0100, Michael S. Tsirkin wrote: > > I think a transitional device should report version 1. > > In legacy drivers version is checked before features are probed > > so as far as I can see there's no way for device to detect a legacy driver and > > switch that to 1 dynamically. > > > > I'm not saying spec should force anyone to implement > > transitional devices, but I don't see why it should > > make this completely impossible. > > > > So I think that a device compliant with the spec can report versions 1 or 2. > > I'll repeat myself. The version register defines the registers layout. > Different register layout, different version number. And let me put this > thing clear - you will not convince my otherwise. You can vote me wrong. > > Pawel > Two questions: - What's wrong with feature bits as a way to select a different layout? That's what they were designed for. - So what's the plan for transitional devices? -- MST

On Fri, 2013-10-11 at 14:36 +0100, Michael S. Tsirkin wrote: > Two questions: > - What's wrong with feature bits as a way to > select a different layout? That's what they were > designed for. The MMIO transport control register layout is not going to change in runtime. It must be either version 1 or version 2. > - So what's the plan for transitional devices? I am not planning anything special in the MMIO transport. Pawel

On Fri, Oct 11, 2013 at 02:42:40PM +0100, Pawel Moll wrote: > On Fri, 2013-10-11 at 14:36 +0100, Michael S. Tsirkin wrote: > > Two questions: > > - What's wrong with feature bits as a way to > > select a different layout? That's what they were > > designed for. > > The MMIO transport control register layout is not going to change in > runtime. It must be either version 1 or version 2. > > > - So what's the plan for transitional devices? > > I am not planning anything special in the MMIO transport. > > Pawel > I see, so MMIO won't allow support transitional devices then. OK, let's add a paragraph making this explicit then.

"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). >> >> Pawel > > 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.

Pawel Moll <pawel.moll@arm.com> writes: > On Fri, 2013-10-04 at 04:36 +0100, Rusty Russell wrote: >> +• 0x018 R ConfigGeneration >> + Configuration atomicity value. >> + Changes every time the configuration noticeably changes. This means the >> + device may only change the value after a configuration read operation, >> + but it must change if there is any risk of a device seeing an inconsistent >> + configuration state. > > How about moving it closer to the config space? Something like 0x0f0 or > even 0x0fc? Sure... I tried to be mininally disruptive. Seems like you're proposing a heavily revised non-legacy format anyway, so slot it wherever. >> +2.3.2.2.1. Legacy Interface: MMIO Device Layout >> +-------------------------- >> +The ConfigGeneration field does not exist in legacy devices; fortunately >> +it would return 0 if accessed. > > Not on "my" models, no. It would result in external abort (think > SIGBUS). > > Not a big problem - the device version number will be bumped to 2 anyway > (working on it right now). Ah, OK. Cheers, Rusty.