* [PATCH v2 1/2] virtio: unexport virtio_finalize_features @ 2022-01-18 17:03 Michael S. Tsirkin 2022-01-18 17:03 ` [PATCH v2 2/2] virtio: acknowledge all features before access Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2022-01-18 17:03 UTC (permalink / raw) To: linux-kernel; +Cc: Cornelia Huck, virtualization virtio_finalize_features is only used internally within virtio. No reason to export it. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Cornelia Huck <cohuck@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio.c | 3 +-- include/linux/virtio.h | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 00ac9db792a4..d891b0a354b0 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -166,7 +166,7 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) } EXPORT_SYMBOL_GPL(virtio_add_status); -int virtio_finalize_features(struct virtio_device *dev) +static int virtio_finalize_features(struct virtio_device *dev) { int ret = dev->config->finalize_features(dev); unsigned status; @@ -202,7 +202,6 @@ int virtio_finalize_features(struct virtio_device *dev) } return 0; } -EXPORT_SYMBOL_GPL(virtio_finalize_features); void virtio_reset_device(struct virtio_device *dev) { diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 72292a62cd90..5464f398912a 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -133,7 +133,6 @@ bool is_virtio_device(struct device *dev); void virtio_break_device(struct virtio_device *dev); void virtio_config_changed(struct virtio_device *dev); -int virtio_finalize_features(struct virtio_device *dev); #ifdef CONFIG_PM_SLEEP int virtio_device_freeze(struct virtio_device *dev); int virtio_device_restore(struct virtio_device *dev); -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] virtio: acknowledge all features before access 2022-01-18 17:03 [PATCH v2 1/2] virtio: unexport virtio_finalize_features Michael S. Tsirkin @ 2022-01-18 17:03 ` Michael S. Tsirkin 2022-01-19 2:52 ` Jason Wang 2022-01-20 14:35 ` Cornelia Huck 0 siblings, 2 replies; 5+ messages in thread From: Michael S. Tsirkin @ 2022-01-18 17:03 UTC (permalink / raw) To: linux-kernel; +Cc: Halil Pasic, Cornelia Huck, stable, virtualization 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. To fix, split finalize_features from virtio_finalize_features and call finalize_features with all feature bits before validation, and then - if validation changed any bits - once again after. Since virtio_finalize_features no longer writes out features rename it to virtio_features_ok - since that is what it does: checks that features are ok with the device. 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 (which is uncommon). 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> fixup! virtio: acknowledge all features before access --- drivers/virtio/virtio.c | 39 ++++++++++++++++++++--------------- include/linux/virtio_config.h | 3 ++- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index d891b0a354b0..d6396be0ea83 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -166,14 +166,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) } EXPORT_SYMBOL_GPL(virtio_add_status); -static int virtio_finalize_features(struct virtio_device *dev) +/* Do some validation, then set FEATURES_OK */ +static int virtio_features_ok(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 +243,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,13 +253,26 @@ 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; + + /* Did validation change any features? Then write them again. */ + if (features != dev->features) { + err = dev->config->finalize_features(dev); + if (err) + goto err; + } } - err = virtio_finalize_features(dev); + err = virtio_features_ok(dev); if (err) goto err; @@ -495,7 +496,11 @@ int virtio_device_restore(struct virtio_device *dev) /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); - ret = virtio_finalize_features(dev); + ret = dev->config->finalize_features(dev); + if (ret) + goto err; + + ret = virtio_features_ok(dev); if (ret) goto err; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 4d107ad31149..dafdc7f48c01 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -64,8 +64,9 @@ struct virtio_shm_region { * Returns the first 64 feature bits (all we currently need). * @finalize_features: confirm what device features we'll be using. * vdev: the virtio_device - * This gives the final feature bits for the device: it can change + * This sends the driver feature bits to the device: it can change * the dev->feature bits if it wants. + * Note: despite the name this can be called any number of times. * Returns 0 on success or error status * @bus_name: return the bus name associated with the device (optional) * vdev: the virtio_device -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] virtio: acknowledge all features before access 2022-01-18 17:03 ` [PATCH v2 2/2] virtio: acknowledge all features before access Michael S. Tsirkin @ 2022-01-19 2:52 ` Jason Wang 2022-01-19 9:18 ` Michael S. Tsirkin 2022-01-20 14:35 ` Cornelia Huck 1 sibling, 1 reply; 5+ messages in thread From: Jason Wang @ 2022-01-19 2:52 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Halil Pasic, Cornelia Huck, linux-kernel, stable, virtualization On Wed, Jan 19, 2022 at 1:04 AM 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. So I guess this is for this part of the spec 3.1.1: """ 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it. """ If it is, is this better to quote in the change log? Other than this, Acked-by: Jason Wang <jasowang@redhat.com> > > To fix, split finalize_features from virtio_finalize_features and > call finalize_features with all feature bits before validation, > and then - if validation changed any bits - once again after. > > Since virtio_finalize_features no longer writes out features > rename it to virtio_features_ok - since that is what it does: > checks that features are ok with the device. > > 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 (which is uncommon). > > 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> > > fixup! virtio: acknowledge all features before access > --- > drivers/virtio/virtio.c | 39 ++++++++++++++++++++--------------- > include/linux/virtio_config.h | 3 ++- > 2 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index d891b0a354b0..d6396be0ea83 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -166,14 +166,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) > } > EXPORT_SYMBOL_GPL(virtio_add_status); > > -static int virtio_finalize_features(struct virtio_device *dev) > +/* Do some validation, then set FEATURES_OK */ > +static int virtio_features_ok(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 +243,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,13 +253,26 @@ 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; > + > + /* Did validation change any features? Then write them again. */ > + if (features != dev->features) { > + err = dev->config->finalize_features(dev); > + if (err) > + goto err; > + } > } > > - err = virtio_finalize_features(dev); > + err = virtio_features_ok(dev); > if (err) > goto err; > > @@ -495,7 +496,11 @@ int virtio_device_restore(struct virtio_device *dev) > /* We have a driver! */ > virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > - ret = virtio_finalize_features(dev); > + ret = dev->config->finalize_features(dev); > + if (ret) > + goto err; > + > + ret = virtio_features_ok(dev); > if (ret) > goto err; > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > index 4d107ad31149..dafdc7f48c01 100644 > --- a/include/linux/virtio_config.h > +++ b/include/linux/virtio_config.h > @@ -64,8 +64,9 @@ struct virtio_shm_region { > * Returns the first 64 feature bits (all we currently need). > * @finalize_features: confirm what device features we'll be using. > * vdev: the virtio_device > - * This gives the final feature bits for the device: it can change > + * This sends the driver feature bits to the device: it can change > * the dev->feature bits if it wants. > + * Note: despite the name this can be called any number of times. > * Returns 0 on success or error status > * @bus_name: return the bus name associated with the device (optional) > * vdev: the virtio_device > -- > MST > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] virtio: acknowledge all features before access 2022-01-19 2:52 ` Jason Wang @ 2022-01-19 9:18 ` Michael S. Tsirkin 0 siblings, 0 replies; 5+ messages in thread From: Michael S. Tsirkin @ 2022-01-19 9:18 UTC (permalink / raw) To: Jason Wang Cc: Halil Pasic, Cornelia Huck, linux-kernel, stable, virtualization On Wed, Jan 19, 2022 at 10:52:34AM +0800, Jason Wang wrote: > On Wed, Jan 19, 2022 at 1:04 AM 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. > > So I guess this is for this part of the spec 3.1.1: > > """ > 4. Read device feature bits, and write the subset of feature bits > understood by the OS and driver to the device. During this step the > driver MAY read (but MUST NOT write) the device-specific configuration > fields to check that it can support the device before accepting it. > """ > > If it is, is this better to quote in the change log? I don't think this spec actually clarifies anything. Sent some spec patches to improve the situation. > Other than this, > > Acked-by: Jason Wang <jasowang@redhat.com> > > > > > To fix, split finalize_features from virtio_finalize_features and > > call finalize_features with all feature bits before validation, > > and then - if validation changed any bits - once again after. > > > > Since virtio_finalize_features no longer writes out features > > rename it to virtio_features_ok - since that is what it does: > > checks that features are ok with the device. > > > > 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 (which is uncommon). > > > > 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> > > > > fixup! virtio: acknowledge all features before access > > --- > > drivers/virtio/virtio.c | 39 ++++++++++++++++++++--------------- > > include/linux/virtio_config.h | 3 ++- > > 2 files changed, 24 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index d891b0a354b0..d6396be0ea83 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -166,14 +166,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) > > } > > EXPORT_SYMBOL_GPL(virtio_add_status); > > > > -static int virtio_finalize_features(struct virtio_device *dev) > > +/* Do some validation, then set FEATURES_OK */ > > +static int virtio_features_ok(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 +243,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,13 +253,26 @@ 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; > > + > > + /* Did validation change any features? Then write them again. */ > > + if (features != dev->features) { > > + err = dev->config->finalize_features(dev); > > + if (err) > > + goto err; > > + } > > } > > > > - err = virtio_finalize_features(dev); > > + err = virtio_features_ok(dev); > > if (err) > > goto err; > > > > @@ -495,7 +496,11 @@ int virtio_device_restore(struct virtio_device *dev) > > /* We have a driver! */ > > virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > > > - ret = virtio_finalize_features(dev); > > + ret = dev->config->finalize_features(dev); > > + if (ret) > > + goto err; > > + > > + ret = virtio_features_ok(dev); > > if (ret) > > goto err; > > > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > > index 4d107ad31149..dafdc7f48c01 100644 > > --- a/include/linux/virtio_config.h > > +++ b/include/linux/virtio_config.h > > @@ -64,8 +64,9 @@ struct virtio_shm_region { > > * Returns the first 64 feature bits (all we currently need). > > * @finalize_features: confirm what device features we'll be using. > > * vdev: the virtio_device > > - * This gives the final feature bits for the device: it can change > > + * This sends the driver feature bits to the device: it can change > > * the dev->feature bits if it wants. > > + * Note: despite the name this can be called any number of times. > > * Returns 0 on success or error status > > * @bus_name: return the bus name associated with the device (optional) > > * vdev: the virtio_device > > -- > > MST > > > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] virtio: acknowledge all features before access 2022-01-18 17:03 ` [PATCH v2 2/2] virtio: acknowledge all features before access Michael S. Tsirkin 2022-01-19 2:52 ` Jason Wang @ 2022-01-20 14:35 ` Cornelia Huck 1 sibling, 0 replies; 5+ messages in thread From: Cornelia Huck @ 2022-01-20 14:35 UTC (permalink / raw) To: Michael S. Tsirkin, linux-kernel; +Cc: Halil Pasic, stable, virtualization On Tue, Jan 18 2022, "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. > > To fix, split finalize_features from virtio_finalize_features and > call finalize_features with all feature bits before validation, > and then - if validation changed any bits - once again after. > > Since virtio_finalize_features no longer writes out features > rename it to virtio_features_ok - since that is what it does: > checks that features are ok with the device. > > 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 (which is uncommon). > > 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> > > fixup! virtio: acknowledge all features before access Leftover from rebasing? > --- > drivers/virtio/virtio.c | 39 ++++++++++++++++++++--------------- > include/linux/virtio_config.h | 3 ++- > 2 files changed, 24 insertions(+), 18 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com> Would like to see a quick sanity test from Halil, though. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-20 14:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-18 17:03 [PATCH v2 1/2] virtio: unexport virtio_finalize_features Michael S. Tsirkin 2022-01-18 17:03 ` [PATCH v2 2/2] virtio: acknowledge all features before access Michael S. Tsirkin 2022-01-19 2:52 ` Jason Wang 2022-01-19 9:18 ` Michael S. Tsirkin 2022-01-20 14:35 ` Cornelia Huck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).