Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932386AbaKMJLv (ORCPT ); Thu, 13 Nov 2014 04:11:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42895 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932074AbaKMJLq (ORCPT ); Thu, 13 Nov 2014 04:11:46 -0500 Message-ID: <546475C2.50606@redhat.com> Date: Thu, 13 Nov 2014 17:11:30 +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: Cornelia Huck CC: rusty@rustcorp.com.au, mst@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH 1/2] virito: introduce methods of fixing device features References: <1415857974-23326-1-git-send-email-jasowang@redhat.com> <20141113094604.1f248c3d.cornelia.huck@de.ibm.com> In-Reply-To: <20141113094604.1f248c3d.cornelia.huck@de.ibm.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/13/2014 04:46 PM, Cornelia Huck wrote: > On Thu, 13 Nov 2014 13:52:53 +0800 > Jason Wang wrote: > > typo in subject-prefix: s/virito/virtio/ > Will correct this. >> Buggy host may advertised buggy host features (a usual case is that host >> advertise a feature whose dependencies were missed). In this case, driver >> should detect and disable the buggy features by itself. >> >> This patch introduces driver specific fix_features() method which is called >> just before features finalizing to detect and disable buggy features >> advertised by host. > So the basic problem this patch fixes is that an individual driver may > only specify a static set of features but cannot specify any > dependencies, right? Right, and what even worse is qemu could not handle dependencies as well. So we need fix both sides. > Adding a sanitizer step makes sense, I guess. > >> Virtio-net will be the first user. >> >> Cc: Rusty Russell >> Cc: Michael S. Tsirkin >> Signed-off-by: Jason Wang >> --- >> drivers/virtio/virtio.c | 4 ++++ >> include/linux/virtio.h | 1 + >> include/linux/virtio_config.h | 12 ++++++++++++ >> 3 files changed, 17 insertions(+) >> >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> index df598dd..7001d6e 100644 >> --- a/drivers/virtio/virtio.c >> +++ b/drivers/virtio/virtio.c >> @@ -181,6 +181,10 @@ static int virtio_dev_probe(struct device *_d) >> if (device_features & (1 << i)) >> set_bit(i, dev->features); >> >> + /* Fix buggy features advertised by host */ >> + if (drv->fix_features) >> + drv->fix_features(dev); > I'd probably call this "sanitize_features" instead. Ok. >> + >> dev->config->finalize_features(dev); >> >> err = drv->probe(dev); >> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h >> index 7f4ef66..7bd89ea 100644 >> --- a/include/linux/virtio_config.h >> +++ b/include/linux/virtio_config.h >> @@ -96,6 +96,18 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, >> return test_bit(fbit, vdev->features); >> } >> >> +static inline void virtio_disable_feature(struct virtio_device *vdev, >> + unsigned int fbit) >> +{ >> + BUG_ON(fbit >= VIRTIO_TRANSPORT_F_START); >> + BUG_ON(vdev->config->get_status(vdev) & >> + ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)); > When we add virtio-1 support, we can add a check for FEATURES_OK here, > so we're really on the safe side. > If I read the spec correctly, FEATURES_OK was set only after writing the features bits to device. But we want to sanitize the them before. >> + >> + virtio_check_driver_offered_feature(vdev, fbit); >> + >> + clear_bit(fbit, vdev->features); >> +} >> + >> static inline >> struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev, >> vq_callback_t *c, const char *n) > The approach looks good to me. > Thanks for the review. -- 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/