Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751078AbaKTF3X (ORCPT ); Thu, 20 Nov 2014 00:29:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37153 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750776AbaKTF3V (ORCPT ); Thu, 20 Nov 2014 00:29:21 -0500 Message-ID: <546D7C1B.7040502@redhat.com> Date: Thu, 20 Nov 2014 13:28:59 +0800 From: Jason Wang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: "Michael S. Tsirkin" CC: rusty@rustcorp.com.au, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Cornelia Huck , Wanlong Gao Subject: Re: [PATCH net] virtio-net: validate features during probe References: <1416378939-28821-1-git-send-email-jasowang@redhat.com> <20141119085939.GB24827@redhat.com> <546C63E6.7040600@redhat.com> <20141119093901.GB26119@redhat.com> In-Reply-To: <20141119093901.GB26119@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/19/2014 05:39 PM, Michael S. Tsirkin wrote: > On Wed, Nov 19, 2014 at 05:33:26PM +0800, Jason Wang wrote: >> On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote: >>> On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang 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 >>>> Cc: Michael S. Tsirkin >>>> Cc: Cornelia Huck >>>> Cc: Wanlong Gao >>>> Signed-off-by: Jason Wang >>>> --- >>>> drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 93 insertions(+) >>>> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>> index ec2a8b4..4a0ad46 100644 >>>> --- a/drivers/net/virtio_net.c >>>> +++ b/drivers/net/virtio_net.c >>>> @@ -1673,6 +1673,95 @@ 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)) { >>>> + 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", >>> This line's way too long. >> Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be >> split) [...] >>>> + >>>> static int virtnet_probe(struct virtio_device *vdev) >>>> { >>>> int i, err; >>>> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev) >>>> struct virtnet_info *vi; >>>> u16 max_queue_pairs; >>>> >>>> + err = virtnet_check_features(vdev); >>>> + if (err) >>>> + return -EINVAL; >>>> + >>>> /* Find if host supports multiqueue virtio_net device */ >>>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ, >>>> struct virtio_net_config, >>> The API seems too complex, and you still had to open-code ECN logic. >>> Just open-code most of it. >> Yes, the ECN could be done through the same way as ctrl_vq did. >> >> How about move those checking into virtio core by just letting device >> export its dependency table? > So far we only have this for net, let's not add > one-off APIs. > >>> You can use a helper macro to output a >>> friendly message without code duplication. >>> For example like the below (completely untested)? >>> >>> >>> I would also like to split things: dependencies on >>> VIRTIO_NET_F_CTRL_VQ might go into this kernel, >>> since they help fix BUG. >>> >>> Others should wait since they don't fix any crashes, and there's a small >>> chance of a regression for some hypervisor in the field. >> Probably ok but not sure, since the rest features are all related to >> csum and offloading, we are in fact depends on network core to fix them. > Well it does fix them so ... there's no bug, is there? > No. >>> --> >>> >>> virtio-net: friendlier handling of misconfigured hypervisors >>> >>> We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ >>> is not set but one of features depending on it is. >>> That's not a friendly way to report errors to >>> hypervisors. >>> Let's check, and fail probe instead. >>> >>> Signed-off-by: Michael S. Tsirkin >>> >>> --- >>> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>> index 26e1330..7a7d1a3 100644 >>> --- a/drivers/net/virtio_net.c >>> +++ b/drivers/net/virtio_net.c >>> @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = { >>> }; >>> #endif >>> >>> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit, >>> + const char *fname) >>> +{ >>> + if (!virtio_has_feature(vdev, fbit)) >>> + return false; >>> + >>> + dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n", >>> + fbit, fname); >>> + >>> + return true; >>> +} >>> + >>> +#define VIRTNET_FAIL_ON(vdev, fbit) \ >>> + __virtnet_fail_on_feature(vdev, fbit, #fbit) >>> + >>> static int virtnet_probe(struct virtio_device *vdev) >>> { >>> int i, err; >>> @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev) >>> struct virtnet_info *vi; >>> u16 max_queue_pairs; >>> >>> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) && >>> + (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) || >>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) || >>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) || >>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) || >>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR))) >>> + return -EINVAL; >>> + >>> /* Find if host supports multiqueue virtio_net device */ >>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ, >>> struct virtio_net_config, >>> >> Patch looks good, but consider we may check more dependencies in the >> future, may need a helper instead. Or just use this and switch to >> dependency table in 3.19. > Pls note this is just pseudo-code - I didn't even compile it. > If you want something like this merged, go ahead and > post it, probably addressing Cornelia's request to > print the dependency too. Maybe: Ok, will post v3. > >>> (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX, "ctrl_vq") || >>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN, "ctrl_vq") || >>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE, "ctrl_vq") || >>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, "ctrl_vq") || >>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR, "ctrl_vq"))) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/