virtio_finalize_features is only used internally within virtio.
No reason to export it.
Signed-off-by: Michael S. Tsirkin <[email protected]>
Reviewed-by: Cornelia Huck <[email protected]>
Acked-by: Jason Wang <[email protected]>
---
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
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: [email protected]
Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
Cc: "Halil Pasic" <[email protected]>
Signed-off-by: Michael S. Tsirkin <[email protected]>
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
On Wed, Jan 19, 2022 at 1:04 AM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>
> 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: [email protected]
> Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> Cc: "Halil Pasic" <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
>
> 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
>
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 <[email protected]> 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 <[email protected]>
>
> >
> > 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: [email protected]
> > Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> > Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> > Cc: "Halil Pasic" <[email protected]>
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> >
> > 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
On Tue, Jan 18 2022, "Michael S. Tsirkin" <[email protected]> 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: [email protected]
> Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> Cc: "Halil Pasic" <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
>
> 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 <[email protected]>
Would like to see a quick sanity test from Halil, though.