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

[Marcel Apfelbaum]

On 09/09/2016 01:55 PM, Cornelia Huck wrote:
> 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?

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