Halil Pasic
2021-Oct-06 14:25 UTC
[PATCH 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: 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 regression so let's try to address it. Signed-off-by: Halil Pasic <pasic at linux.ibm.com> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space") Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range") Reported-by: markver at us.ibm.com --- drivers/virtio/virtio.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 0a5b54034d4b..494cfecd3376 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -239,6 +239,16 @@ 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 for these when needed. + */ + if (drv->validate && !virtio_legacy_is_little_endian() + && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) { + 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 -- 2.25.1
Cornelia Huck
2021-Oct-07 11:52 UTC
[PATCH 1/1] virtio: write back F_VERSION_1 before validate
On Wed, Oct 06 2021, Halil Pasic <pasic at linux.ibm.com> wrote:> 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: driver MAY read (but MUST NOT > write) the device-specific configuration fields to check that it can > support the device before setting FEATURES_OK.Suggest to put the citations from the spec into quotes, so that they are distinguishable from the rest of the text.> > 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 regression so let's try to addresss/regression/a regression/> it.Maybe mention what the regression is? Also mention that we use this workaround for modern on BE only?> > Signed-off-by: Halil Pasic <pasic at linux.ibm.com> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space") > Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range") > Reported-by: markver at us.ibm.com > --- > drivers/virtio/virtio.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 0a5b54034d4b..494cfecd3376 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -239,6 +239,16 @@ 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 for these when needed."...to force LE config space accesses before FEATURES_OK for these when needed (BE)." ?> + */ > + if (drv->validate && !virtio_legacy_is_little_endian() > + && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {Nit: putting device_features first would read more naturally to me.> + 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; > elsePatch LGTM.