On Wed, 19 Nov 2014 15:21:29 +0800 Jason Wang <[email protected]> wrote:
> This patch validates feature dependencies during probe and fail the probing > if a dependency is missed. This fixes the issues of hitting BUG() > when qemu fails to advertise features correctly. One example is booting > guest with ctrl_vq=off through qemu. > > Cc: Rusty Russell <[email protected]> > Cc: Michael S. Tsirkin <[email protected]> > Cc: Cornelia Huck <[email protected]> > Cc: Wanlong Gao <[email protected]> > Signed-off-by: Jason Wang <[email protected]> > --- > Changes from V1: > - Drop VIRTIO_NET_F_*_UFO from the checklist, since it was disabled > --- > drivers/net/virtio_net.c | 91 > ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index ec2a8b4..b16a761 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1673,6 +1673,93 @@ static const struct attribute_group > virtio_net_mrg_rx_group = { > }; > #endif > > +static int virtnet_validate_features(struct virtio_device *dev, > + unsigned int *table, > + int table_size, > + unsigned int feature) > +{ > + int i; > + > + if (!virtio_has_feature(dev, feature)) { Do an early return, get rid of one indentation level? > + for (i = 0; i < table_size; i++) { > + unsigned int f = table[i]; > + > + if (virtio_has_feature(dev, f)) { > + dev_err(&dev->dev, > + "buggy hyperviser: feature 0x%x was > advertised but its dependency 0x%x was not", s/hyperviser/hypervisor/ (also below) > + f, feature); > + return -EINVAL; > + } > + } > + } > + > + return 0; > +} > + > +static int virtnet_check_features(struct virtio_device *dev) > +{ > + unsigned int features_for_ctrl_vq[] = { > + VIRTIO_NET_F_CTRL_RX, > + VIRTIO_NET_F_CTRL_VLAN, > + VIRTIO_NET_F_GUEST_ANNOUNCE, > + VIRTIO_NET_F_MQ, > + VIRTIO_NET_F_CTRL_MAC_ADDR > + }; > + unsigned int features_for_guest_csum[] = { > + VIRTIO_NET_F_GUEST_TSO4, > + VIRTIO_NET_F_GUEST_TSO6, > + VIRTIO_NET_F_GUEST_ECN, > + }; > + unsigned int features_for_host_csum[] = { > + VIRTIO_NET_F_HOST_TSO4, > + VIRTIO_NET_F_HOST_TSO6, > + VIRTIO_NET_F_HOST_ECN, > + }; I'm wondering whether it would be easier to read if you listed all prereqs per feature instead of all features that depend on a feature? It would still be hard to express the v4/v6 or conditions below in tables, though. Or call the arrays features_depending_on_foo? > + int err; > + > + err = virtnet_validate_features(dev, features_for_ctrl_vq, > + ARRAY_SIZE(features_for_ctrl_vq), > + VIRTIO_NET_F_CTRL_VQ); > + if (err) > + return err; If you already print a message that a user may use to fix their hypervisor (or bug someone about it), would it make sense to check all dependencies and print a full list of everything that is broken in the advertised feature bits? I usually hate it if I fix one thing only to hit the next bug when the program could have already told me about everything I need to fix :) > + > + err = virtnet_validate_features(dev, features_for_guest_csum, > + ARRAY_SIZE(features_for_guest_csum), > + VIRTIO_NET_F_GUEST_CSUM); > + if (err) > + return err; > + > + err = virtnet_validate_features(dev, features_for_host_csum, > + ARRAY_SIZE(features_for_host_csum), > + VIRTIO_NET_F_CSUM); > + if (err) > + return err; > + > + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) && > + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) || > + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) { > + dev_err(&dev->dev, > + "buggy hyperviser: feature 0x%x was advertised but its > dependency 0x%x or 0x%x was not", > + VIRTIO_NET_F_GUEST_ECN, > + VIRTIO_NET_F_GUEST_TSO4, > + VIRTIO_NET_F_GUEST_TSO6); > + return -EINVAL; > + } > + > + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) && > + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) || > + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) { > + dev_err(&dev->dev, > + "buggy hyperviser: feature 0x%x was advertised but its > dependency 0x%x or 0x%x was not", "Hypervisor bug: advertised feature <foo> but not <bar> or <baz>" ? > + VIRTIO_NET_F_HOST_ECN, > + VIRTIO_NET_F_HOST_TSO4, > + VIRTIO_NET_F_HOST_TSO6); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int virtnet_probe(struct virtio_device *vdev) > { > int i, err; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
