On Mon, Jan 17, 2022 at 02:31:49PM +0800, Jason Wang wrote:

在 2022/1/15 上午4:09, Michael S. Tsirkin 写道:

...

The feature negotiation was designed in a way that makes it possible for devices to know which config fields will be accessed by drivers.

This is broken since commit 404123c2db79 ("virtio: allow drivers to validate features") with fallout in at least block and net. We have a partial work-around in commit 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") which at least lets devices find out which format should config space have, but this is a partial fix: guests should not access config space without acknowledging features since otherwise we'll never be able to change the config space format.

As a side effect, this also reduces the amount of hypervisor accesses - we now only acknowledge features once unless we are clearing any features when validating.

Cc: stable@vger.kernel.org Fixes: 404123c2db79 ("virtio: allow drivers to validate features") Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") Cc: "Halil Pasic" pasic@linux.ibm.com Signed-off-by: Michael S. Tsirkin mst@redhat.com

---

Halil, I thought hard about our situation with transitional and today I finally thought of something I am happy with. Pls let me know what you think. Testing on big endian would also be much appreciated!

drivers/virtio/virtio.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index d891b0a354b0..2ed6e2451fd8 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status); static int virtio_finalize_features(struct virtio_device *dev) {

• int ret = dev->config->finalize_features(dev); unsigned status;

• int ret; might_sleep();

• if (ret)

• return ret;
  

  ret = arch_has_restricted_virtio_memory_access(); if (ret) {

@@ -244,17 +242,6 @@ 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

@@ -265,10 +252,22 @@ static int virtio_dev_probe(struct device *_d) if (device_features & (1ULL << i)) __virtio_set_bit(dev, i);

• err = dev->config->finalize_features(dev);
• if (err)

• goto err;
  

• if (drv->validate) {

• u64 features = dev->features;
  

• err = drv->validate(dev); if (err) goto err;

• if (features != dev->features) {
  

• 	err = dev->config->finalize_features(dev);
  

• 	if (err)
  

• 		goto err;
  

• }
  

  } err = virtio_finalize_features(dev);

@@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);

• ret = dev->config->finalize_features(dev);
• if (ret)

• goto err;
  

Is this part of code related?

Thanks

Yes. virtio_finalize_features no longer calls dev->config->finalize_features.

I think the dev->config->finalize_features callback is actually a misnomer now, it just sends the features to device, finalize is FEATURES_OK. Renaming that is a bigger patch though, and I'd like this one to be cherry-pickable to stable.

...

• ret = virtio_finalize_features(dev); if (ret) goto err;

On Mon, Jan 17, 2022 at 01:38:42PM +0100, Cornelia Huck wrote:

On Mon, Jan 17 2022, "Michael S. Tsirkin" mst@redhat.com wrote:

...

On Mon, Jan 17, 2022 at 02:31:49PM +0800, Jason Wang wrote:

...

在 2022/1/15 上午4:09, Michael S. Tsirkin 写道:

...

@@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);

• ret = dev->config->finalize_features(dev);
• if (ret)

• goto err;
  

Is this part of code related?

Thanks

Yes. virtio_finalize_features no longer calls dev->config->finalize_features.

I think the dev->config->finalize_features callback is actually a misnomer now, it just sends the features to device, finalize is FEATURES_OK. Renaming that is a bigger patch though, and I'd like this one to be cherry-pickable to stable.

Do we want to add a comment before the calls to ->finalize_features() (/* write features to device */) and adapt the comment in virtio_ring.h? Should still be stable-friendly, and giving the callback a better name can be a follow-up patch.

Sounds like a good idea. I can also document that near virtio_finalize_features.

...

...

...

• ret = virtio_finalize_features(dev); if (ret) goto err;

On Tue, Jan 18 2022, "Michael S. Tsirkin" mst@redhat.com wrote:

On Mon, Jan 17, 2022 at 01:38:42PM +0100, Cornelia Huck wrote:

...

On Mon, Jan 17 2022, "Michael S. Tsirkin" mst@redhat.com wrote:

...

On Mon, Jan 17, 2022 at 02:31:49PM +0800, Jason Wang wrote:

...

在 2022/1/15 上午4:09, Michael S. Tsirkin 写道:

...

@@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);

• ret = dev->config->finalize_features(dev);
• if (ret)

• goto err;
  

Is this part of code related?

Thanks

Yes. virtio_finalize_features no longer calls dev->config->finalize_features.

I think the dev->config->finalize_features callback is actually a misnomer now, it just sends the features to device, finalize is FEATURES_OK. Renaming that is a bigger patch though, and I'd like this one to be cherry-pickable to stable.

Do we want to add a comment before the calls to ->finalize_features() (/* write features to device */) and adapt the comment in virtio_ring.h? Should still be stable-friendly, and giving the callback a better name can be a follow-up patch.

Sorry which comment in virtio_ring.h? Could not find anything.

Typo; I ment virtio_config.h...

...

...

...

...

• ret = virtio_finalize_features(dev); if (ret) goto err;

On Tue, Jan 18, 2022 at 01:48:55PM +0100, Halil Pasic wrote:

On Fri, 14 Jan 2022 15:09:14 -0500 "Michael S. Tsirkin" mst@redhat.com wrote:

...

The feature negotiation was designed in a way that makes it possible for devices to know which config fields will be accessed by drivers.

This is broken since commit 404123c2db79 ("virtio: allow drivers to validate features") with fallout in at least block and net. We have a partial work-around in commit 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") which at least lets devices find out which format should config space have, but this is a partial fix: guests should not access config space without acknowledging features since otherwise we'll never be able to change the config space format.

As a side effect, this also reduces the amount of hypervisor accesses - we now only acknowledge features once unless we are clearing any features when validating.

Cc: stable@vger.kernel.org Fixes: 404123c2db79 ("virtio: allow drivers to validate features") Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") Cc: "Halil Pasic" pasic@linux.ibm.com Signed-off-by: Michael S. Tsirkin mst@redhat.com

---

Halil, I thought hard about our situation with transitional and today I finally thought of something I am happy with. Pls let me know what you think. Testing on big endian would also be much appreciated!

Hi Michael!

I was just about to have a look into this. But it does not apply cleanly to Linus master (fetched a couple of minutes ago). I also tride with d9679d0013a66849~1 but no luck. What is a suitable base for this patch?

Regards, Halil

It's on top of virtio: unexport virtio_finalize_features You can also get commit cc1f7f0bb64302c1153aa9337db970e6360b379d (HEAD, kernel.org/vhost, kernel.org/linux-next) from my tree.

...

drivers/virtio/virtio.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index d891b0a354b0..2ed6e2451fd8 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status); static int virtio_finalize_features(struct virtio_device *dev) {

• int ret = dev->config->finalize_features(dev); unsigned status;

• int ret;

might_sleep();

• if (ret)

• return ret;
  

ret = arch_has_restricted_virtio_memory_access(); if (ret) { @@ -244,17 +242,6 @@ 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

@@ -265,10 +252,22 @@ static int virtio_dev_probe(struct device *_d) if (device_features & (1ULL << i)) __virtio_set_bit(dev, i);

• err = dev->config->finalize_features(dev);
• if (err)

• goto err;
  

• if (drv->validate) {

• u64 features = dev->features;
  

• err = drv->validate(dev); if (err) goto err;

• if (features != dev->features) {
  

• 	err = dev->config->finalize_features(dev);
  

• 	if (err)
  

• 		goto err;
  

• }
  

  }

err = virtio_finalize_features(dev); @@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);

• ret = dev->config->finalize_features(dev);
• if (ret)

• goto err;
  

• ret = virtio_finalize_features(dev); if (ret) goto err;

On Tue, Jan 18, 2022 at 03:43:50PM +0100, Halil Pasic wrote:

On Fri, 14 Jan 2022 15:09:14 -0500 "Michael S. Tsirkin" mst@redhat.com wrote:

...

The feature negotiation was designed in a way that makes it possible for devices to know which config fields will be accessed by drivers.

This is broken since commit 404123c2db79 ("virtio: allow drivers to validate features") with fallout in at least block and net. We have a partial work-around in commit 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") which at least lets devices find out which format should config space have, but this is a partial fix: guests should not access config space without acknowledging features since otherwise we'll never be able to change the config space format.

I agree with that. The crux is what does "acknowledge features" exactly mean. Is it "write features" or "complete the feature negotiation, including setting FEATURES_OK".

Right. I think originally it was "write features". We then added the FEATURES_OK in order to give devices a chance to reject a set of features, and while doing this we failed to notice that at this point "acknowledge" became confusing.

There's a spec proposal to make things more explicit, and I think I will tweak it to actually use the term "acknowledge".

On top of that, it makes sense to go back, scan all of the spec and see whether any places that we changed from set not negotiated for clarify actually should read "set in the written".

My understanding is, that we should not rely on that the device is going to act according to the negotiated feature set unless FEATURES_OK was set successfully.

not for writes, but for reads there's little choice.

That would mean, that this change ain't guaranteed to help with the stated problem. We simply don't know if the fact that features were written is going to have a side-effect or not. Also see below.

right. at the moment it's just the MTU field. In the future it can be more, but by that future time we can fix the spec ;)

...

As a side effect, this also reduces the amount of hypervisor accesses - we now only acknowledge features once unless we are clearing any features when validating.

My understanding is that this patch basically does for all the features, what commit 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") did only for F_VERSION_1 and under certain conditions to be minimally invasive.

I don't like when s390 is the oddball, so I'm very happy to see us moving away from that.

...

Cc: stable@vger.kernel.org Fixes: 404123c2db79 ("virtio: allow drivers to validate features") Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") Cc: "Halil Pasic" pasic@linux.ibm.com Signed-off-by: Michael S. Tsirkin mst@redhat.com

---

Halil, I thought hard about our situation with transitional and today I finally thought of something I am happy with. Pls let me know what you think. Testing on big endian would also be much appreciated!

Thanks! I will first provide some comments, and I intend to come back with the test results later.

...

drivers/virtio/virtio.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index d891b0a354b0..2ed6e2451fd8 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status); static int virtio_finalize_features(struct virtio_device *dev) {

• int ret = dev->config->finalize_features(dev); unsigned status;

• int ret;

might_sleep();

• if (ret)

• return ret;
  

ret = arch_has_restricted_virtio_memory_access(); if (ret) { @@ -244,17 +242,6 @@ 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

@@ -265,10 +252,22 @@ static int virtio_dev_probe(struct device *_d) if (device_features & (1ULL << i)) __virtio_set_bit(dev, i);

• err = dev->config->finalize_features(dev);

A side note: config->finalize_features() ain't the best name for what the thing does. After config->finalize_features() the features are not final. Unlike after virtio_finalize_features(). IMHO filter_and_write_features() would be a more accurate, although longer name.

I agree. But I think I will start with figuring out the best name in the spec. Maybe write back or acknowledge. Let's see what gets accepted by the tc. Then I'll change code to match.

After this point, the features aren't final yet, and one can not say that a some feature X has been negotiated. But with regards to features, the spec does not really consider this limbo state.

Should this change? Do we want to say: the device SHOULD pick up, and act upon the new features *before* FEATURES_OK is set?

Yes but only for handling config reads. See my spec proposal.

...

...

• if (err)

• goto err;
  

• if (drv->validate) {

• u64 features = dev->features;
  

• err = drv->validate(dev);

... Consider the "we would like to introduce a new config space format" example. Here, I guess we would like to use the new format. But let's say _F_CFG_FMT_V2 aint negotiated yet. So to be sure about the format, we would need to specify, that the behavior of the device needs to change after the feature has been written, but before FEATURES_OK is set, at least for _F_CFG_FMT_V2.

for config space reads. yes.

Please also consider the QEMU implementation of the vhost-user stuff. We push the features to the back-end only when FEATURES_OK status is written.

that has to change, anyway - it's needed to fix endian-ness.

...

if (err)
	goto err;

• if (features != dev->features) {
  

• 	err = dev->config->finalize_features(dev);
  

It is fine to call it again, because the features aren't finalized yet. And re-doing any transport-level filtering and validation is fine as well.

Will add a comment.

...

• 	if (err)
  

• 		goto err;
  

• }
  

  }

err = virtio_finalize_features(dev);

Here the features are finally negotiated and final.

...

@@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);

• ret = dev->config->finalize_features(dev);
• if (ret)

• goto err;
  

• ret = virtio_finalize_features(dev);

Looks a little weird, because virtio_finalize_features() used to include filter + write + set FEATURES_OK. But it ain't too bad.

Better names would benefit readability though, if we can come up with some.

Regards, Halil

right. I think that can be a patch on top though.

...

if (ret) goto err;