Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Cornelia Huck]

On Fri, 9 Sep 2016 13:48:00 +0300
Marcel Apfelbaum <address@hidden> wrote:

> On 09/09/2016 01:33 PM, Cornelia Huck wrote:
> > On Fri,  9 Sep 2016 12:14:31 +0200
> > Maxime Coquelin <address@hidden> 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 <address@hidden>
> >> Cc: Michael S. Tsirkin <address@hidden>
> >> Cc: address@hidden
> >> Signed-off-by: Maxime Coquelin <address@hidden>
> >> ---
> >>  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?