| Message ID | 1473416072-7063-2-git-send-email-maxime.coquelin@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 9 Sep 2016 12:14:31 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> 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 <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: qemu-stable@nongnu.org > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > 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.
On 09/09/2016 01:33 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 12:14:31 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> 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 <marcel@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> 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? Thanks, Marcel
On Fri, 9 Sep 2016 13:48:00 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 09/09/2016 01:33 PM, Cornelia Huck wrote: > > On Fri, 9 Sep 2016 12:14:31 +0200 > > Maxime Coquelin <maxime.coquelin@redhat.com> 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 <marcel@redhat.com> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Cc: qemu-stable@nongnu.org > >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >> --- > >> 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?
On 09/09/2016 01:55 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 13:48:00 +0300 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> On 09/09/2016 01:33 PM, Cornelia Huck wrote: >>> On Fri, 9 Sep 2016 12:14:31 +0200 >>> Maxime Coquelin <maxime.coquelin@redhat.com> 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 <marcel@redhat.com> >>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> --- >>>> 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
On Fri, 9 Sep 2016 14:02:17 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > 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. I think you'll still end up with the same problem (overindicating unsupportedness), as you start out with an otherwise empty feature set, causing features with dependencies to be removed. I fear that anything starting with an incomplete feature list will have that problem. Maybe it would be better to limit this to the one bit we currently want to test (VERSION_1)? We know the semantics of that one. Less general, but also less headaches.
On 09/09/2016 01:20 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 14:02:17 +0300 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> 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. > > I think you'll still end up with the same problem (overindicating > unsupportedness), as you start out with an otherwise empty feature > set, causing features with dependencies to be removed. I fear that > anything starting with an incomplete feature list will have that > problem. > > Maybe it would be better to limit this to the one bit we currently want > to test (VERSION_1)? We know the semantics of that one. Less general, > but also less headaches. Yes, that could be the solution. I agree that making it too generic might create confusion with some features. Thanks, Maxime
On 09/09/2016 02:36 PM, Maxime Coquelin wrote: > > > On 09/09/2016 01:20 PM, Cornelia Huck wrote: >> On Fri, 9 Sep 2016 14:02:17 +0300 >> Marcel Apfelbaum <marcel@redhat.com> wrote: >> >>> 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. >> >> I think you'll still end up with the same problem (overindicating >> unsupportedness), as you start out with an otherwise empty feature >> set, causing features with dependencies to be removed. I fear that >> anything starting with an incomplete feature list will have that >> problem. >> >> Maybe it would be better to limit this to the one bit we currently want >> to test (VERSION_1)? We know the semantics of that one. Less general, >> but also less headaches. > > Yes, that could be the solution. > I agree that making it too generic might create confusion > with some features. > Sounds good to me. Let's go with it and we'll re-think it if we'll find other feature negotiation issues later. Thanks, Marcel > Thanks, > Maxime >
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); diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index d2490c1..5fb74c8 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -235,6 +235,8 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val); void virtio_reset(void *opaque); void virtio_update_irq(VirtIODevice *vdev); int virtio_set_features(VirtIODevice *vdev, uint64_t val); +bool virtio_test_backend_feature(VirtIODevice *vdev, + unsigned int fbit, Error **errp); /* Base devices. */ typedef struct VirtIOBlkConf VirtIOBlkConf;
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 <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: qemu-stable@nongnu.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- hw/virtio/virtio.c | 14 ++++++++++++++ include/hw/virtio/virtio.h | 2 ++ 2 files changed, 16 insertions(+)