From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biJTJ-0004qH-4W for qemu-devel@nongnu.org; Fri, 09 Sep 2016 06:55:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biJTE-00049y-1I for qemu-devel@nongnu.org; Fri, 09 Sep 2016 06:55:49 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:42468 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biJTD-00049A-OP for qemu-devel@nongnu.org; Fri, 09 Sep 2016 06:55:43 -0400 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u89Ah29x044457 for ; Fri, 9 Sep 2016 06:55:43 -0400 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0b-001b2d01.pphosted.com with ESMTP id 25bf1542t6-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 09 Sep 2016 06:55:42 -0400 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 9 Sep 2016 11:55:41 +0100 Date: Fri, 9 Sep 2016 12:55:36 +0200 From: Cornelia Huck In-Reply-To: References: <1473416072-7063-1-git-send-email-maxime.coquelin@redhat.com> <1473416072-7063-2-git-send-email-maxime.coquelin@redhat.com> <20160909123348.4779e59c.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-Id: <20160909125536.3af41223.cornelia.huck@de.ibm.com> Subject: Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: Maxime Coquelin , mst@redhat.com, qemu-devel@nongnu.org, vkaplans@redhat.com, qemu-stable@nongnu.org On Fri, 9 Sep 2016 13:48:00 +0300 Marcel Apfelbaum wrote: > On 09/09/2016 01:33 PM, Cornelia Huck wrote: > > On Fri, 9 Sep 2016 12:14:31 +0200 > > Maxime Coquelin wrote: > > > >> This patch adds virtio_test_backend_feature() function to > >> enable checking a backend feature before the negociation > >> takes place. > >> > >> It may be used, for example, to check whether the backend > >> supports VIRTIO_F_VERSION_1 before enabling modern > >> capabilities. > >> > >> Cc: Marcel Apfelbaum > >> Cc: Michael S. Tsirkin > >> Cc: qemu-stable@nongnu.org > >> Signed-off-by: Maxime Coquelin > >> --- > >> hw/virtio/virtio.c | 14 ++++++++++++++ > >> include/hw/virtio/virtio.h | 2 ++ > >> 2 files changed, 16 insertions(+) > >> > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> index 74c085c..7ab91a1 100644 > >> --- a/hw/virtio/virtio.c > >> +++ b/hw/virtio/virtio.c > >> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size) > >> virtio_save(VIRTIO_DEVICE(opaque), f); > >> } > >> > >> +bool virtio_test_backend_feature(VirtIODevice *vdev, > >> + unsigned int fbit, Error **errp) > >> +{ > >> + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > >> + uint64_t feature; > >> + > >> + virtio_add_feature(&feature, fbit); > >> + > >> + assert(k->get_features != NULL); > >> + feature = k->get_features(vdev, feature, errp); > >> + > >> + return virtio_has_feature(feature, fbit); > >> +} > >> + > >> static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val) > >> { > >> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > What happens if you want to test for features that depend upon each > > other? The backend may support your feature, but it may withdraw the > > feature bit if a dependency is not fullfilled. > > > > You'll probably want to run validation on the whole feature set; but > > that is hard if you're too early in the setup process. > > > > While I agree with the feature dependency issue , would the negation be ok? > What I mean is: if the backend does not support feature X, no > matter what the depending features are, it will still not support it after the negotiation. > > Changing the function to virtio_backend_unsupported_feature(x) would be better? I think yes, although that would mean we need a new query function that pokes through all the layers, no?