From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56490) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biJZl-0006az-0V for qemu-devel@nongnu.org; Fri, 09 Sep 2016 07:02:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biJZf-0006iM-0G for qemu-devel@nongnu.org; Fri, 09 Sep 2016 07:02:28 -0400 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> <20160909125536.3af41223.cornelia.huck@de.ibm.com> From: Marcel Apfelbaum Message-ID: <74bd31bc-a4ce-295a-b138-f9ae872a618a@redhat.com> Date: Fri, 9 Sep 2016 14:02:17 +0300 MIME-Version: 1.0 In-Reply-To: <20160909125536.3af41223.cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Cornelia Huck Cc: Maxime Coquelin , mst@redhat.com, qemu-devel@nongnu.org, vkaplans@redhat.com, qemu-stable@nongnu.org On 09/09/2016 01:55 PM, Cornelia Huck wrote: > 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? > I was thinking to keep the same function proposed by Maxime and change it to negate things: /* * A missing feature before all negotiations finished will still be missing at the end. */ bool virtio_test_backend_unsupported_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); } We only check if the feature was not there from the start. Thanks, Marcel