virtio: write back F_VERSION_1 before validate

PATCH Superseded Halil Pasic 3 participants 5 messages Oct 8, 2021

Subsystems (3)

Stable branch s390 virtio core

lore

Share & tools

lore

Halil Pasic

author

2021-10-08 12:34 UTC #0 lore raw

PATCH v2 1/1 virtio: write back F_VERSION_1 before validate

The virtio specification virtio-v1.1-cs01 states: "Transitional devices MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not been acknowledged by the driver." This is exactly what QEMU as of 6.1 has done relying solely on VIRTIO_F_VERSION_1 for detecting that.

However, the specification also says: "... the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device ..." before setting FEATURES_OK.

In that case, any transitional device relying solely on VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in legacy format. In particular, this implies that it is in big endian format for big endian guests. This naturally confuses the driver which expects little endian in the modern mode.

It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. However, we already have a regression so let's try to address it.

The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio 1.0 is used on both sides. The latter renders virtio-blk unusable with DASD backing, because things simply don't work with the default.

Cc <[email protected]> #v4.11

Signed-off-by Halil Pasic <[email protected]>

Fixes 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")

Fixes fe36cbe0671e ("virtio_net: clear MTU when out of range")

Reported-by [email protected]

---
 drivers/virtio/virtio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/virtio/virtio.c +11 -0

index 0a5b54034d4b..236081afe9a2 100644
@@ -239,6 +239,17 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
+	/*
+	 * Some devices detect legacy solely via F_VERSION_1. Write
+	 * F_VERSION_1 to force LE config space accesses before FEATURES_OK for
+	 * these when needed.
+	 */
+	if (drv->validate && !virtio_legacy_is_little_endian()
+			  && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) {
+		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
+		dev->config->finalize_features(dev);
+	}
+
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else

base-commit: 60a9483534ed0d99090a2ee1d4bb0b8179195f51

Signature · 6 lines

2.25.1

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Michael S. Tsirkin

maintainer

2021-10-08 13:05 UTC #1 lore raw

Quoted 15 lines · Halil Pasic

The virtio specification virtio-v1.1-cs01 states: "Transitional devices MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not been acknowledged by the driver." This is exactly what QEMU as of 6.1 has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
However, the specification also says: "... the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device ..." before setting FEATURES_OK.
In that case, any transitional device relying solely on VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in legacy format. In particular, this implies that it is in big endian format for big endian guests. This naturally confuses the driver which expects little endian in the modern mode.

Quoted 3 lines

It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. However, we already have a regression so let's try to address

actually, regressions. and we can add "since originally before validate callback existed config space was only read after FEATURES_OK. See Fixes tags for relevant commits"

Quoted 6 lines

it.
The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio 1.0 is used on both sides. The latter renders virtio-blk unusable with DASD backing, because things simply don't work with the default.

Let's add a work around description now:

For QEMU, we can work around the issue by writing out the features register with VIRTIO_F_VERSION_1 bit set. We (ab) use the finalize_features config op for this. It's not enough to address vhost user and vhost block devices since these do not get the features until FEATURES_OK, however it looks like these two actually never handled the endian-ness for legacy mode correctly, so at least that's not a regression.

No devices except virtio net and virtio blk seem to be affected.

Long term the right thing to do is to fix the hypervisors.

Quoted 33 lines

Cc <[email protected]> #v4.11

Signed-off-by Halil Pasic <[email protected]>

Fixes 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")

Fixes fe36cbe0671e ("virtio_net: clear MTU when out of range")

Reported-by [email protected]

---
 drivers/virtio/virtio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/virtio/virtio.c +11 -0

index 0a5b54034d4b..236081afe9a2 100644
@@ -239,6 +239,17 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
+	/*
+	 * Some devices detect legacy solely via F_VERSION_1. Write
+	 * F_VERSION_1 to force LE config space accesses before FEATURES_OK for
+	 * these when needed.
+	 */
+	if (drv->validate && !virtio_legacy_is_little_endian()
+			  && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) {
+		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
+		dev->config->finalize_features(dev);
+	}
+
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else

Quoted 3 lines

base-commit: 60a9483534ed0d99090a2ee1d4bb0b8179195f51
Signature · 1 line
2.25.1

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Halil Pasic

author

2021-10-08 13:52 UTC #2 lore raw

Quoted 16 lines · On Fri, 8 Oct 2021 09:05:03 -0400 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 15 lines · Halil Pasic
The virtio specification virtio-v1.1-cs01 states: "Transitional devices MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not been acknowledged by the driver." This is exactly what QEMU as of 6.1 has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
However, the specification also says: "... the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device ..." before setting FEATURES_OK.
In that case, any transitional device relying solely on VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in legacy format. In particular, this implies that it is in big endian format for big endian guests. This naturally confuses the driver which expects little endian in the modern mode.

Quoted 10 lines

Quoted 3 lines
It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. However, we already have a regression so let's try to address
actually, regressions. and we can add "since originally before validate callback existed config space was only read after FEATURES_OK. See Fixes tags for relevant commits"
Quoted 1 line
it.

How about replacing the paragraph above with the following?

"It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. Before validate callback existed, config space was only read after FEATURES_OK. However, we already have two regression, so let's address this here as well."

Quoted 6 lines

Quoted 6 lines
The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio 1.0 is used on both sides. The latter renders virtio-blk unusable with DASD backing, because things simply don't work with the default.

and add
"See Fixes tags for relevant commits."
here.

Quoted 6 lines

Let's add a work around description now:
For QEMU, we can work around the issue by writing out the features register with VIRTIO_F_VERSION_1 bit set. We (ab) use the

s/features register/feature bits/
rationale: ccw does not have a features register, and qemu does not really act as if its behavior was controlled by the values in a features register. I.e. when we read the register we see VIRTIO_F_VERSION_! because the feature is offered. In QEMU we basically read host_featues but write the guest_features. And what drives device behavior is mostly guest_features.

s/(ab) use/(ab)use/

Quoted 1 line

finalize_features config op for this. It's not enough to address vhost

s/It's/This is/

Quoted 1 line

user and vhost block devices since these do not get the features until

s/vhost user and vhost block/some vhost-user and vhost-vdpa/ ?

Ratioale: I think vhost block is just a vhost-user device. On the other hand vhost-user-fs works like charm because the config space is implemented in qemu and not in the vhost-user device. I didn't check vhost_net. I'm not even sure qemu offers a vhost_net implementation. Anyway I wouldn't like to make any false statements here.

Quoted 8 lines

FEATURES_OK, however it looks like these two actually never handled the endian-ness for legacy mode correctly, so at least that's not a regression.
No devices except virtio net and virtio blk seem to be affected.
Long term the right thing to do is to fix the hypervisors.

Sounds good. Thanks! Are you OK with my changes proposed to your changes?

Regards,
Halil

Quoted 36 lines

Quoted 36 lines
Cc <[email protected]> #v4.11
Signed-off-by Halil Pasic <[email protected]>
Fixes 82e89ea077b9 ("virtio-blk: Add validation for block size in
config space") Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range") Reported-by: [email protected]
---
 drivers/virtio/virtio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/virtio/virtio.c +3 -0
index 0a5b54034d4b..236081afe9a2 100644
@@ -239,6 +239,17 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
+	/*
+	 * Some devices detect legacy solely via F_VERSION_1. Write
+	 * F_VERSION_1 to force LE config space accesses before
FEATURES_OK for
drivers/virtio/virtio.c +4 -0
+	 * these when needed.
+	 */
+	if (drv->validate && !virtio_legacy_is_little_endian()
+			  && device_features &
BIT_ULL(VIRTIO_F_VERSION_1)) {
drivers/virtio/virtio.c +4 -0
+		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
+		dev->config->finalize_features(dev);
+	}
+
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else

Quoted 4 lines

Quoted 3 lines
base-commit: 60a9483534ed0d99090a2ee1d4bb0b8179195f51
Signature · 1 line
2.25.1  

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Cornelia Huck

Reviewed-by

2021-10-08 15:08 UTC #3 lore raw

Quoted 126 lines · Halil Pasic

Quoted 16 lines · On Fri, 8 Oct 2021 09:05:03 -0400 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 15 lines · Halil Pasic
The virtio specification virtio-v1.1-cs01 states: "Transitional devices MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not been acknowledged by the driver." This is exactly what QEMU as of 6.1 has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
However, the specification also says: "... the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device ..." before setting FEATURES_OK.
In that case, any transitional device relying solely on VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in legacy format. In particular, this implies that it is in big endian format for big endian guests. This naturally confuses the driver which expects little endian in the modern mode.
Quoted 10 lines
Quoted 3 lines
It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. However, we already have a regression so let's try to address
actually, regressions. and we can add "since originally before validate callback existed config space was only read after FEATURES_OK. See Fixes tags for relevant commits"
Quoted 1 line
it.
How about replacing the paragraph above with the following?
"It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. Before validate callback existed, config space was only read after FEATURES_OK. However, we already have two regression, so let's address this here as well."
Quoted 6 lines
Quoted 6 lines
The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio 1.0 is used on both sides. The latter renders virtio-blk unusable with DASD backing, because things simply don't work with the default.
and add
"See Fixes tags for relevant commits."
here.
Quoted 6 lines
Let's add a work around description now:
For QEMU, we can work around the issue by writing out the features register with VIRTIO_F_VERSION_1 bit set. We (ab) use the
s/features register/feature bits/
rationale: ccw does not have a features register, and qemu does not really act as if its behavior was controlled by the values in a features register. I.e. when we read the register we see VIRTIO_F_VERSION_! because the feature is offered. In QEMU we basically read host_featues but write the guest_features. And what drives device behavior is mostly guest_features.
s/(ab) use/(ab)use/
Quoted 1 line
finalize_features config op for this. It's not enough to address vhost
s/It's/This is/
Quoted 1 line
user and vhost block devices since these do not get the features until
s/vhost user and vhost block/some vhost-user and vhost-vdpa/ ?
Ratioale: I think vhost block is just a vhost-user device. On the other hand vhost-user-fs works like charm because the config space is implemented in qemu and not in the vhost-user device. I didn't check vhost_net. I'm not even sure qemu offers a vhost_net implementation. Anyway I wouldn't like to make any false statements here.
Quoted 8 lines
FEATURES_OK, however it looks like these two actually never handled the endian-ness for legacy mode correctly, so at least that's not a regression.
No devices except virtio net and virtio blk seem to be affected.
Long term the right thing to do is to fix the hypervisors.
Sounds good. Thanks! Are you OK with my changes proposed to your changes?
Regards,
Halil
Quoted 36 lines
Quoted 36 lines
Cc <[email protected]> #v4.11
Signed-off-by Halil Pasic <[email protected]>
Fixes 82e89ea077b9 ("virtio-blk: Add validation for block size in
config space") Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range") Reported-by: [email protected]
---
 drivers/virtio/virtio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/virtio/virtio.c +3 -0
index 0a5b54034d4b..236081afe9a2 100644
@@ -239,6 +239,17 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
+	/*
+	 * Some devices detect legacy solely via F_VERSION_1. Write
+	 * F_VERSION_1 to force LE config space accesses before
FEATURES_OK for
drivers/virtio/virtio.c +4 -0
+	 * these when needed.
+	 */
+	if (drv->validate && !virtio_legacy_is_little_endian()
+			  && device_features &
BIT_ULL(VIRTIO_F_VERSION_1)) {
drivers/virtio/virtio.c +4 -0
+		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
+		dev->config->finalize_features(dev);
+	}
+
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else

Quoted 1 line

Quoted 1 line
Quoted 1 line
base-commit: 60a9483534ed0d99090a2ee1d4bb0b8179195f51

FWIW, with the amends:

Reviewed-by Cornelia Huck <[email protected]>

Michael S. Tsirkin

maintainer

2021-10-08 15:42 UTC #4 lore raw

In reply to Halil Pasic

Quoted 28 lines · Halil Pasic

Quoted 25 lines · On Fri, 8 Oct 2021 09:05:03 -0400 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 15 lines · Halil Pasic
The virtio specification virtio-v1.1-cs01 states: "Transitional devices MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not been acknowledged by the driver." This is exactly what QEMU as of 6.1 has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
However, the specification also says: "... the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device ..." before setting FEATURES_OK.
In that case, any transitional device relying solely on VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in legacy format. In particular, this implies that it is in big endian format for big endian guests. This naturally confuses the driver which expects little endian in the modern mode.
Quoted 3 lines
It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. However, we already have a regression so let's try to address
actually, regressions. and we can add "since originally before validate callback existed config space was only read after FEATURES_OK. See Fixes tags for relevant commits"

Quoted 8 lines

Quoted 1 line
Quoted 1 line
it.
How about replacing the paragraph above with the following?
"It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. Before validate callback existed, config space was only read after FEATURES_OK. However, we already have two regression,

two regressions

Quoted 28 lines

so
let's address this here as well."
Quoted 6 lines
Quoted 6 lines
The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio 1.0 is used on both sides. The latter renders virtio-blk unusable with DASD backing, because things simply don't work with the default.
and add
"See Fixes tags for relevant commits."
here.
Quoted 6 lines
Let's add a work around description now:
For QEMU, we can work around the issue by writing out the features register with VIRTIO_F_VERSION_1 bit set. We (ab) use the
s/features register/feature bits/
rationale: ccw does not have a features register, and qemu does not really act as if its behavior was controlled by the values in a features register. I.e. when we read the register we see VIRTIO_F_VERSION_! because the feature is offered. In QEMU we basically read host_featues but write the guest_features. And what drives device behavior is mostly guest_features.
s/(ab) use/(ab)use/

Quoted 7 lines

Quoted 1 line
finalize_features config op for this. It's not enough to address vhost
s/It's/This is/
Quoted 1 line
user and vhost block devices since these do not get the features until
s/vhost user and vhost block/some vhost-user and vhost-vdpa/ ?

Let's just say "not enough to address vhost devices since some of these etc"

Quoted 5 lines

Ratioale: I think vhost block is just a vhost-user device. On the other hand vhost-user-fs works like charm because the config space is implemented in qemu and not in the vhost-user device. I didn't check vhost_net. I'm not even sure qemu offers a vhost_net implementation.

it does

Quoted 1 line

Anyway I wouldn't like to make any false statements here.

Quoted 6 lines

Quoted 6 lines
FEATURES_OK, however it looks like these two actually never handled the endian-ness for legacy mode correctly, so at least that's not a regression.
No devices except virtio net and virtio blk seem to be affected.

Quoted 7 lines

Quoted 2 lines
Long term the right thing to do is to fix the hypervisors.
Sounds good. Thanks! Are you OK with my changes proposed to your changes?
Regards,
Halil

yes.

Quoted 36 lines

Quoted 36 lines
Quoted 36 lines
Cc <[email protected]> #v4.11
Signed-off-by Halil Pasic <[email protected]>
Fixes 82e89ea077b9 ("virtio-blk: Add validation for block size in
config space") Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range") Reported-by: [email protected]
---
 drivers/virtio/virtio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/virtio/virtio.c +3 -0
index 0a5b54034d4b..236081afe9a2 100644
@@ -239,6 +239,17 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
+	/*
+	 * Some devices detect legacy solely via F_VERSION_1. Write
+	 * F_VERSION_1 to force LE config space accesses before
FEATURES_OK for
drivers/virtio/virtio.c +4 -0
+	 * these when needed.
+	 */
+	if (drv->validate && !virtio_legacy_is_little_endian()
+			  && device_features &
BIT_ULL(VIRTIO_F_VERSION_1)) {
drivers/virtio/virtio.c +4 -0
+		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
+		dev->config->finalize_features(dev);
+	}
+
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
base-commit: 60a9483534ed0d99090a2ee1d4bb0b8179195f51
Signature · 1 line
2.25.1  

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization