* [Qemu-devel] [PATCH 0/2] virtio-pci: Improve device plugging whith legacy backends @ 2016-09-09 10:14 Maxime Coquelin 2016-09-09 10:14 ` [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality Maxime Coquelin 2016-09-09 10:14 ` [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 Maxime Coquelin 0 siblings, 2 replies; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 10:14 UTC (permalink / raw) To: mst, qemu-devel; +Cc: marcel, vkaplans, qemu-stable, Maxime Coquelin This series makes device plugging more robust, to avoid guest to be confused when the backend doesn't support VIRTIO_F_VERSION_1. The problem is seen with Linux guests running mainline kernels, when backend doesn't support the feature: virtio_net virtio0: virtio: device uses modern interface but does not have VIRTIO_F_VERSION_1. When it happens, the modern device probe returns -EINVAL, whereas its caller expects -ENODEV being returned to switch to legacy device probing. We need to make QEMU more robust to ensure the guest won't be confused, so this series exposes modern interface only when backend support it. It has been tested with vhost-net and vhost-user backends in client and server modes. Maxime Coquelin (2): virtio: Add backend feature testing functionnality virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 hw/virtio/virtio-pci.c | 15 +++++++++++++++ hw/virtio/virtio-pci.h | 5 +++++ hw/virtio/virtio.c | 14 ++++++++++++++ include/hw/virtio/virtio.h | 2 ++ 4 files changed, 36 insertions(+) -- 2.7.4 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 10:14 [Qemu-devel] [PATCH 0/2] virtio-pci: Improve device plugging whith legacy backends Maxime Coquelin @ 2016-09-09 10:14 ` Maxime Coquelin 2016-09-09 10:33 ` Cornelia Huck 2016-09-09 10:14 ` [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 Maxime Coquelin 1 sibling, 1 reply; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 10:14 UTC (permalink / raw) To: mst, qemu-devel; +Cc: marcel, vkaplans, qemu-stable, Maxime Coquelin 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); 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 10:14 ` [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality Maxime Coquelin @ 2016-09-09 10:33 ` Cornelia Huck 2016-09-09 10:48 ` Marcel Apfelbaum 0 siblings, 1 reply; 20+ messages in thread From: Cornelia Huck @ 2016-09-09 10:33 UTC (permalink / raw) To: Maxime Coquelin; +Cc: mst, qemu-devel, marcel, vkaplans, qemu-stable 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 10:33 ` Cornelia Huck @ 2016-09-09 10:48 ` Marcel Apfelbaum 2016-09-09 10:55 ` Cornelia Huck 0 siblings, 1 reply; 20+ messages in thread From: Marcel Apfelbaum @ 2016-09-09 10:48 UTC (permalink / raw) To: Cornelia Huck, Maxime Coquelin; +Cc: mst, qemu-devel, vkaplans, qemu-stable 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 10:48 ` Marcel Apfelbaum @ 2016-09-09 10:55 ` Cornelia Huck 2016-09-09 11:02 ` Marcel Apfelbaum 0 siblings, 1 reply; 20+ messages in thread From: Cornelia Huck @ 2016-09-09 10:55 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: Maxime Coquelin, mst, qemu-devel, vkaplans, qemu-stable 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? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 10:55 ` Cornelia Huck @ 2016-09-09 11:02 ` Marcel Apfelbaum 2016-09-09 11:20 ` Cornelia Huck 0 siblings, 1 reply; 20+ messages in thread From: Marcel Apfelbaum @ 2016-09-09 11:02 UTC (permalink / raw) To: Cornelia Huck; +Cc: Maxime Coquelin, mst, qemu-devel, vkaplans, qemu-stable 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 11:02 ` Marcel Apfelbaum @ 2016-09-09 11:20 ` Cornelia Huck 2016-09-09 11:36 ` Maxime Coquelin 0 siblings, 1 reply; 20+ messages in thread From: Cornelia Huck @ 2016-09-09 11:20 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: Maxime Coquelin, mst, qemu-devel, vkaplans, qemu-stable 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 11:20 ` Cornelia Huck @ 2016-09-09 11:36 ` Maxime Coquelin 2016-09-09 11:47 ` Marcel Apfelbaum 0 siblings, 1 reply; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 11:36 UTC (permalink / raw) To: Cornelia Huck, Marcel Apfelbaum; +Cc: mst, qemu-devel, vkaplans, qemu-stable 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality 2016-09-09 11:36 ` Maxime Coquelin @ 2016-09-09 11:47 ` Marcel Apfelbaum 0 siblings, 0 replies; 20+ messages in thread From: Marcel Apfelbaum @ 2016-09-09 11:47 UTC (permalink / raw) To: Maxime Coquelin, Cornelia Huck; +Cc: mst, qemu-devel, vkaplans, qemu-stable 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 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 10:14 [Qemu-devel] [PATCH 0/2] virtio-pci: Improve device plugging whith legacy backends Maxime Coquelin 2016-09-09 10:14 ` [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality Maxime Coquelin @ 2016-09-09 10:14 ` Maxime Coquelin 2016-09-09 10:40 ` Cornelia Huck 1 sibling, 1 reply; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 10:14 UTC (permalink / raw) To: mst, qemu-devel; +Cc: marcel, vkaplans, qemu-stable, Maxime Coquelin This patch makes pci devices plugging more robust, by not confusing guest with modern interface when the backend doesn't support VIRTIO_F_VERSION_1. 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-pci.c | 15 +++++++++++++++ hw/virtio/virtio-pci.h | 5 +++++ 2 files changed, 20 insertions(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 755f921..0e5d59c 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) uint32_t size; VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + /* + * Virtio capabilities present without + * VIRTIO_F_VERSION_1 confuses guests + */ + if (!virtio_test_backend_feature(vdev, VIRTIO_F_VERSION_1, errp)) { + virtio_pci_disable_modern(proxy); + } + + legacy = virtio_pci_legacy(proxy); + modern = virtio_pci_modern(proxy); + if (!legacy && !modern) { + error_setg(errp, "PCI device is neither legacy nor modern."); + return; + } + config = proxy->pci_dev.config; if (proxy->class_code) { pci_config_set_class(config, proxy->class_code); diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 25fbf8a..4e976b3 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -172,6 +172,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) proxy->disable_legacy = ON_OFF_AUTO_ON; } +static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) +{ + proxy->disable_modern = true; +} + /* * virtio-scsi-pci: This extends VirtioPCIProxy. */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 10:14 ` [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 Maxime Coquelin @ 2016-09-09 10:40 ` Cornelia Huck 2016-09-09 11:04 ` Marcel Apfelbaum 0 siblings, 1 reply; 20+ messages in thread From: Cornelia Huck @ 2016-09-09 10:40 UTC (permalink / raw) To: Maxime Coquelin; +Cc: mst, qemu-devel, marcel, vkaplans, qemu-stable On Fri, 9 Sep 2016 12:14:32 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > This patch makes pci devices plugging more robust, by not confusing > guest with modern interface when the backend doesn't support > VIRTIO_F_VERSION_1. > > 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-pci.c | 15 +++++++++++++++ > hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 20 insertions(+) Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for ccw") fixes this issue for ccw via the introduction of a ->post_plugged() callback. Unfortunately, we did not find a good way to make it work for pci back then. Two comments: - ->post_plugged() handles the dependencies (as noticed for your first patch) - and this is due to it being called after the plugging is already done. - I don't really like pci and ccw being too different. We have probably more flexibility with the handling for ccw, so I could probably convert ccw to the same mechanism that pci uses. Maybe there are other uses for ->post_plugged()? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 10:40 ` Cornelia Huck @ 2016-09-09 11:04 ` Marcel Apfelbaum 2016-09-09 11:20 ` Cornelia Huck 0 siblings, 1 reply; 20+ messages in thread From: Marcel Apfelbaum @ 2016-09-09 11:04 UTC (permalink / raw) To: Cornelia Huck, Maxime Coquelin; +Cc: mst, qemu-devel, vkaplans, qemu-stable On 09/09/2016 01:40 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 12:14:32 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >> This patch makes pci devices plugging more robust, by not confusing >> guest with modern interface when the backend doesn't support >> VIRTIO_F_VERSION_1. >> >> 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-pci.c | 15 +++++++++++++++ >> hw/virtio/virtio-pci.h | 5 +++++ >> 2 files changed, 20 insertions(+) > > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for > ccw") fixes this issue for ccw via the introduction of a > ->post_plugged() callback. Unfortunately, we did not find a good way to > make it work for pci back then. It seems that for ccw is enough to rewind dev->rev_max, sadly for pci we need to rewind a lot of settings/resources. Thanks, Marcel > > Two comments: > - ->post_plugged() handles the dependencies (as noticed for your first > patch) - and this is due to it being called after the plugging is > already done. > - I don't really like pci and ccw being too different. We have probably > more flexibility with the handling for ccw, so I could probably convert > ccw to the same mechanism that pci uses. Maybe there are other uses for > ->post_plugged()? > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 11:04 ` Marcel Apfelbaum @ 2016-09-09 11:20 ` Cornelia Huck 2016-09-09 11:44 ` Maxime Coquelin 0 siblings, 1 reply; 20+ messages in thread From: Cornelia Huck @ 2016-09-09 11:20 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: Maxime Coquelin, mst, qemu-devel, vkaplans, qemu-stable On Fri, 9 Sep 2016 14:04:55 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 09/09/2016 01:40 PM, Cornelia Huck wrote: > > On Fri, 9 Sep 2016 12:14:32 +0200 > > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > > >> This patch makes pci devices plugging more robust, by not confusing > >> guest with modern interface when the backend doesn't support > >> VIRTIO_F_VERSION_1. > >> > >> 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-pci.c | 15 +++++++++++++++ > >> hw/virtio/virtio-pci.h | 5 +++++ > >> 2 files changed, 20 insertions(+) > > > > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for > > ccw") fixes this issue for ccw via the introduction of a > > ->post_plugged() callback. Unfortunately, we did not find a good way to > > make it work for pci back then. > > It seems that for ccw is enough to rewind dev->rev_max, > sadly for pci we need to rewind a lot of settings/resources. Yes, that what I meant with 'more flexibility for ccw'. > > Thanks, > Marcel > > > > > Two comments: > > - ->post_plugged() handles the dependencies (as noticed for your first > > patch) - and this is due to it being called after the plugging is > > already done. > > - I don't really like pci and ccw being too different. We have probably > > more flexibility with the handling for ccw, so I could probably convert > > ccw to the same mechanism that pci uses. Maybe there are other uses for > > ->post_plugged()? > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 11:20 ` Cornelia Huck @ 2016-09-09 11:44 ` Maxime Coquelin 2016-09-09 11:49 ` Cornelia Huck 0 siblings, 1 reply; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 11:44 UTC (permalink / raw) To: Cornelia Huck, Marcel Apfelbaum; +Cc: mst, qemu-devel, vkaplans, qemu-stable On 09/09/2016 01:20 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 14:04:55 +0300 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> On 09/09/2016 01:40 PM, Cornelia Huck wrote: >>> On Fri, 9 Sep 2016 12:14:32 +0200 >>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote: >>> >>>> This patch makes pci devices plugging more robust, by not confusing >>>> guest with modern interface when the backend doesn't support >>>> VIRTIO_F_VERSION_1. >>>> >>>> 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-pci.c | 15 +++++++++++++++ >>>> hw/virtio/virtio-pci.h | 5 +++++ >>>> 2 files changed, 20 insertions(+) >>> >>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for >>> ccw") fixes this issue for ccw via the introduction of a >>> ->post_plugged() callback. Unfortunately, we did not find a good way to >>> make it work for pci back then. >> >> It seems that for ccw is enough to rewind dev->rev_max, >> sadly for pci we need to rewind a lot of settings/resources. > > Yes, that what I meant with 'more flexibility for ccw'. Maybe we could replace post_plugged with a pre_plugged approach? In ->pre_plugged(), cww and pci would specify which features it can support using virtio_add_feature(). Then we could call get_features() before ->device_plugged(). Doing this, both ccw and pci would have the needed information without having to rewind any settings. Does that make sense? But for now, I think it would be better to merge something in the spirit of this series (taking into account to remarks). Indeed, I think we want this fixed in stable, but the above proposal would be too huge for stable. Thanks, Maxime ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 11:44 ` Maxime Coquelin @ 2016-09-09 11:49 ` Cornelia Huck 2016-09-09 12:01 ` Maxime Coquelin 2016-09-09 14:14 ` [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated Maxime Coquelin 0 siblings, 2 replies; 20+ messages in thread From: Cornelia Huck @ 2016-09-09 11:49 UTC (permalink / raw) To: Maxime Coquelin; +Cc: Marcel Apfelbaum, mst, qemu-devel, vkaplans, qemu-stable On Fri, 9 Sep 2016 13:44:35 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > On 09/09/2016 01:20 PM, Cornelia Huck wrote: > > On Fri, 9 Sep 2016 14:04:55 +0300 > > Marcel Apfelbaum <marcel@redhat.com> wrote: > > > >> On 09/09/2016 01:40 PM, Cornelia Huck wrote: > >>> On Fri, 9 Sep 2016 12:14:32 +0200 > >>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >>> > >>>> This patch makes pci devices plugging more robust, by not confusing > >>>> guest with modern interface when the backend doesn't support > >>>> VIRTIO_F_VERSION_1. > >>>> > >>>> 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-pci.c | 15 +++++++++++++++ > >>>> hw/virtio/virtio-pci.h | 5 +++++ > >>>> 2 files changed, 20 insertions(+) > >>> > >>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for > >>> ccw") fixes this issue for ccw via the introduction of a > >>> ->post_plugged() callback. Unfortunately, we did not find a good way to > >>> make it work for pci back then. > >> > >> It seems that for ccw is enough to rewind dev->rev_max, > >> sadly for pci we need to rewind a lot of settings/resources. > > > > Yes, that what I meant with 'more flexibility for ccw'. > Maybe we could replace post_plugged with a pre_plugged approach? > > In ->pre_plugged(), cww and pci would specify which features it can > support using virtio_add_feature(). > Then we could call get_features() before ->device_plugged(). I think that would work for ccw (haven't looked at pci). > > Doing this, both ccw and pci would have the needed information without > having to rewind any settings. > > Does that make sense? > > But for now, I think it would be better to merge something in the spirit > of this series (taking into account to remarks). > Indeed, I think we want this fixed in stable, but the above proposal > would be too huge for stable. A 'just check for VERSION_1' approach would probably be best for stable. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 2016-09-09 11:49 ` Cornelia Huck @ 2016-09-09 12:01 ` Maxime Coquelin 2016-09-09 14:14 ` [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated Maxime Coquelin 1 sibling, 0 replies; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 12:01 UTC (permalink / raw) To: Cornelia Huck; +Cc: Marcel Apfelbaum, mst, qemu-devel, vkaplans, qemu-stable On 09/09/2016 01:49 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 13:44:35 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >> On 09/09/2016 01:20 PM, Cornelia Huck wrote: >>> On Fri, 9 Sep 2016 14:04:55 +0300 >>> Marcel Apfelbaum <marcel@redhat.com> wrote: >>> >>>> On 09/09/2016 01:40 PM, Cornelia Huck wrote: >>>>> On Fri, 9 Sep 2016 12:14:32 +0200 >>>>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote: >>>>> >>>>>> This patch makes pci devices plugging more robust, by not confusing >>>>>> guest with modern interface when the backend doesn't support >>>>>> VIRTIO_F_VERSION_1. >>>>>> >>>>>> 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-pci.c | 15 +++++++++++++++ >>>>>> hw/virtio/virtio-pci.h | 5 +++++ >>>>>> 2 files changed, 20 insertions(+) >>>>> >>>>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for >>>>> ccw") fixes this issue for ccw via the introduction of a >>>>> ->post_plugged() callback. Unfortunately, we did not find a good way to >>>>> make it work for pci back then. >>>> >>>> It seems that for ccw is enough to rewind dev->rev_max, >>>> sadly for pci we need to rewind a lot of settings/resources. >>> >>> Yes, that what I meant with 'more flexibility for ccw'. >> Maybe we could replace post_plugged with a pre_plugged approach? >> >> In ->pre_plugged(), cww and pci would specify which features it can >> support using virtio_add_feature(). >> Then we could call get_features() before ->device_plugged(). > > I think that would work for ccw (haven't looked at pci). Good, once quick fix accepted, I'll try this solution. > >> >> Doing this, both ccw and pci would have the needed information without >> having to rewind any settings. >> >> Does that make sense? >> >> But for now, I think it would be better to merge something in the spirit >> of this series (taking into account to remarks). >> Indeed, I think we want this fixed in stable, but the above proposal >> would be too huge for stable. > > A 'just check for VERSION_1' approach would probably be best for stable. > Ok, thanks. I will send a v2 replacing the generic function with a VERISON_1 specfic: bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp); Maxime ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated 2016-09-09 11:49 ` Cornelia Huck 2016-09-09 12:01 ` Maxime Coquelin @ 2016-09-09 14:14 ` Maxime Coquelin 2016-09-09 15:39 ` Michael S. Tsirkin 1 sibling, 1 reply; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 14:14 UTC (permalink / raw) To: mst, qemu-devel, cornelia.huck; +Cc: marcel, vkaplans, Maxime Coquelin Currently, devices are plugged before features are negotiated. If the backend doesn't support VIRTIO_F_VERSION_1, the transport need to rewind some settings. This is the case for both PCI and CCW. For CCW, a post_plugged callback had been introduced, where max_rev field is just updated if VIRTIO_F_VERSION_1 is not supported by the backend. For PCI, implementing the post_plugged would be much more complicated, so the current fix consists in checking whether the backend supports VIRTIO_F_VERSION_1 in the backend. This patch propose to replace existing solutions with a common approach by negociating features before ->device_plugged() is called. A pre_plugged callback is introduced so that the transports can set their supported features. Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- Cornelia, This is what I proposed earlier today. I have tested it successfully with PCI. Does it sound good to you? Would you have a try with CCW? Note that this is based on my earlier series which fixes PCI. Thanks, Maxime --- hw/s390x/virtio-ccw.c | 30 +++++++++++++++--------------- hw/virtio/virtio-bus.c | 9 +++++---- hw/virtio/virtio-pci.c | 18 ++++++++++++++---- include/hw/virtio/virtio-bus.h | 10 +++++----- 4 files changed, 39 insertions(+), 28 deletions(-) diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index a554a24..e10a88d 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1261,6 +1261,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) return 0; } +static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) +{ + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); + + if (dev->max_rev >= 1) { + virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); + } +} + /* This is called by virtio-bus just after the device is plugged. */ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) { @@ -1270,6 +1280,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) SubchDev *sch = ccw_dev->sch; int n = virtio_get_num_queues(vdev); + if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { + dev->max_rev = 0; + } + if (virtio_get_num_queues(vdev) > VIRTIO_CCW_QUEUE_MAX) { error_setg(errp, "The number of virtqueues %d " "exceeds ccw limit %d", n, @@ -1283,25 +1297,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus); - if (dev->max_rev >= 1) { - virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); - } css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, d->hotplugged, 1); } -static void virtio_ccw_post_plugged(DeviceState *d, Error **errp) -{ - VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); - VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); - - if (!virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) { - /* A backend didn't support modern virtio. */ - dev->max_rev = 0; - } -} - static void virtio_ccw_device_unplugged(DeviceState *d) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); @@ -1593,8 +1593,8 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data) k->load_queue = virtio_ccw_load_queue; k->save_config = virtio_ccw_save_config; k->load_config = virtio_ccw_load_config; + k->pre_plugged = virtio_ccw_pre_plugged; k->device_plugged = virtio_ccw_device_plugged; - k->post_plugged = virtio_ccw_post_plugged; k->device_unplugged = virtio_ccw_device_unplugged; k->ioeventfd_started = virtio_ccw_ioeventfd_started; k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started; diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index a85b7c8..9d2111f 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -49,16 +49,17 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) DPRINTF("%s: plug device.\n", qbus->name); - if (klass->device_plugged != NULL) { - klass->device_plugged(qbus->parent, errp); + if (klass->pre_plugged != NULL) { + klass->pre_plugged(qbus->parent, errp); } /* Get the features of the plugged device. */ assert(vdc->get_features != NULL); vdev->host_features = vdc->get_features(vdev, vdev->host_features, errp); - if (klass->post_plugged != NULL) { - klass->post_plugged(qbus->parent, errp); + + if (klass->device_plugged != NULL) { + klass->device_plugged(qbus->parent, errp); } } diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 9e88d7b..07e6c60 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1569,6 +1569,18 @@ static void virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy, ®ion->mr); } +static void virtio_pci_pre_plugged(DeviceState *d, Error **errp) +{ + VirtIOPCIProxy *proxy = VIRTIO_PCI(d); + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + + if (virtio_pci_modern(proxy)) { + virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); + } + + virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE); +} + /* This is called by virtio-bus just after the device is plugged. */ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) { @@ -1585,7 +1597,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) * Virtio capabilities present without * VIRTIO_F_VERSION_1 confuses guests */ - if (!virtio_test_backend_virtio_1(vdev, errp)) { + if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { virtio_pci_disable_modern(proxy); } @@ -1637,7 +1649,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) struct virtio_pci_cfg_cap *cfg_mask; - virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); virtio_pci_modern_regions_init(proxy); virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap); @@ -1702,8 +1713,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) if (!kvm_has_many_ioeventfds()) { proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; } - - virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE); } static void virtio_pci_device_unplugged(DeviceState *d) @@ -2459,6 +2468,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) k->query_guest_notifiers = virtio_pci_query_guest_notifiers; k->set_guest_notifiers = virtio_pci_set_guest_notifiers; k->vmstate_change = virtio_pci_vmstate_change; + k->pre_plugged = virtio_pci_pre_plugged; k->device_plugged = virtio_pci_device_plugged; k->device_unplugged = virtio_pci_device_unplugged; k->query_nvectors = virtio_pci_query_nvectors; diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h index f3e5ef3..24caa0a 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -54,16 +54,16 @@ typedef struct VirtioBusClass { int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign); void (*vmstate_change)(DeviceState *d, bool running); /* + * Expose the features the transport layer supports before + * the negotiation takes place. + */ + void (*pre_plugged)(DeviceState *d, Error **errp); + /* * transport independent init function. * This is called by virtio-bus just after the device is plugged. */ void (*device_plugged)(DeviceState *d, Error **errp); /* - * Re-evaluate setup after feature bits have been validated - * by the device backend. - */ - void (*post_plugged)(DeviceState *d, Error **errp); - /* * transport independent exit function. * This is called by virtio-bus just before the device is unplugged. */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated 2016-09-09 14:14 ` [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated Maxime Coquelin @ 2016-09-09 15:39 ` Michael S. Tsirkin 2016-09-09 16:16 ` Maxime Coquelin 0 siblings, 1 reply; 20+ messages in thread From: Michael S. Tsirkin @ 2016-09-09 15:39 UTC (permalink / raw) To: Maxime Coquelin; +Cc: qemu-devel, cornelia.huck, marcel, vkaplans On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote: > Currently, devices are plugged before features are negotiated. > If the backend doesn't support VIRTIO_F_VERSION_1, the transport > need to rewind some settings. > > This is the case for both PCI and CCW. > For CCW, a post_plugged callback had been introduced, where > max_rev field is just updated if VIRTIO_F_VERSION_1 is not > supported by the backend. > For PCI, implementing the post_plugged would be much more > complicated, so the current fix consists in checking whether > the backend supports VIRTIO_F_VERSION_1 in the backend. > > This patch propose to replace existing solutions with a common > approach by negociating features before ->device_plugged() is > called. A pre_plugged callback is introduced so that the > transports can set their supported features. > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Looks good - can you pls test and post a non-RFC? > --- > Cornelia, > > This is what I proposed earlier today. > > I have tested it successfully with PCI. > Does it sound good to you? Would you have a try with CCW? > > Note that this is based on my earlier series which fixes PCI. > > Thanks, > Maxime > --- > hw/s390x/virtio-ccw.c | 30 +++++++++++++++--------------- > hw/virtio/virtio-bus.c | 9 +++++---- > hw/virtio/virtio-pci.c | 18 ++++++++++++++---- > include/hw/virtio/virtio-bus.h | 10 +++++----- > 4 files changed, 39 insertions(+), 28 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index a554a24..e10a88d 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1261,6 +1261,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) > return 0; > } > > +static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) > +{ > + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + > + if (dev->max_rev >= 1) { > + virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); > + } > +} > + > /* This is called by virtio-bus just after the device is plugged. */ > static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) > { > @@ -1270,6 +1280,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) > SubchDev *sch = ccw_dev->sch; > int n = virtio_get_num_queues(vdev); > > + if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { > + dev->max_rev = 0; > + } > + > if (virtio_get_num_queues(vdev) > VIRTIO_CCW_QUEUE_MAX) { > error_setg(errp, "The number of virtqueues %d " > "exceeds ccw limit %d", n, > @@ -1283,25 +1297,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) > > sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus); > > - if (dev->max_rev >= 1) { > - virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); > - } > > css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, > d->hotplugged, 1); > } > > -static void virtio_ccw_post_plugged(DeviceState *d, Error **errp) > -{ > - VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > - VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > - > - if (!virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) { > - /* A backend didn't support modern virtio. */ > - dev->max_rev = 0; > - } > -} > - > static void virtio_ccw_device_unplugged(DeviceState *d) > { > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > @@ -1593,8 +1593,8 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data) > k->load_queue = virtio_ccw_load_queue; > k->save_config = virtio_ccw_save_config; > k->load_config = virtio_ccw_load_config; > + k->pre_plugged = virtio_ccw_pre_plugged; > k->device_plugged = virtio_ccw_device_plugged; > - k->post_plugged = virtio_ccw_post_plugged; > k->device_unplugged = virtio_ccw_device_unplugged; > k->ioeventfd_started = virtio_ccw_ioeventfd_started; > k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started; > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index a85b7c8..9d2111f 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -49,16 +49,17 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > DPRINTF("%s: plug device.\n", qbus->name); > > - if (klass->device_plugged != NULL) { > - klass->device_plugged(qbus->parent, errp); > + if (klass->pre_plugged != NULL) { > + klass->pre_plugged(qbus->parent, errp); > } > > /* Get the features of the plugged device. */ > assert(vdc->get_features != NULL); > vdev->host_features = vdc->get_features(vdev, vdev->host_features, > errp); > - if (klass->post_plugged != NULL) { > - klass->post_plugged(qbus->parent, errp); > + > + if (klass->device_plugged != NULL) { > + klass->device_plugged(qbus->parent, errp); > } > } > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 9e88d7b..07e6c60 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1569,6 +1569,18 @@ static void virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy, > ®ion->mr); > } > > +static void virtio_pci_pre_plugged(DeviceState *d, Error **errp) > +{ > + VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > + > + if (virtio_pci_modern(proxy)) { > + virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); > + } > + > + virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE); > +} > + > /* This is called by virtio-bus just after the device is plugged. */ > static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > { > @@ -1585,7 +1597,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > * Virtio capabilities present without > * VIRTIO_F_VERSION_1 confuses guests > */ > - if (!virtio_test_backend_virtio_1(vdev, errp)) { > + if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { > virtio_pci_disable_modern(proxy); > } > > @@ -1637,7 +1649,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > > struct virtio_pci_cfg_cap *cfg_mask; > > - virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); > virtio_pci_modern_regions_init(proxy); > > virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap); > @@ -1702,8 +1713,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > if (!kvm_has_many_ioeventfds()) { > proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > } > - > - virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE); > } > > static void virtio_pci_device_unplugged(DeviceState *d) > @@ -2459,6 +2468,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) > k->query_guest_notifiers = virtio_pci_query_guest_notifiers; > k->set_guest_notifiers = virtio_pci_set_guest_notifiers; > k->vmstate_change = virtio_pci_vmstate_change; > + k->pre_plugged = virtio_pci_pre_plugged; > k->device_plugged = virtio_pci_device_plugged; > k->device_unplugged = virtio_pci_device_unplugged; > k->query_nvectors = virtio_pci_query_nvectors; > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index f3e5ef3..24caa0a 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -54,16 +54,16 @@ typedef struct VirtioBusClass { > int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign); > void (*vmstate_change)(DeviceState *d, bool running); > /* > + * Expose the features the transport layer supports before > + * the negotiation takes place. > + */ > + void (*pre_plugged)(DeviceState *d, Error **errp); > + /* > * transport independent init function. > * This is called by virtio-bus just after the device is plugged. > */ > void (*device_plugged)(DeviceState *d, Error **errp); > /* > - * Re-evaluate setup after feature bits have been validated > - * by the device backend. > - */ > - void (*post_plugged)(DeviceState *d, Error **errp); > - /* > * transport independent exit function. > * This is called by virtio-bus just before the device is unplugged. > */ > -- > 2.7.4 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated 2016-09-09 15:39 ` Michael S. Tsirkin @ 2016-09-09 16:16 ` Maxime Coquelin 2016-09-09 18:44 ` Michael S. Tsirkin 0 siblings, 1 reply; 20+ messages in thread From: Maxime Coquelin @ 2016-09-09 16:16 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel, cornelia.huck, marcel, vkaplans On 09/09/2016 05:39 PM, Michael S. Tsirkin wrote: > On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote: >> > Currently, devices are plugged before features are negotiated. >> > If the backend doesn't support VIRTIO_F_VERSION_1, the transport >> > need to rewind some settings. >> > >> > This is the case for both PCI and CCW. >> > For CCW, a post_plugged callback had been introduced, where >> > max_rev field is just updated if VIRTIO_F_VERSION_1 is not >> > supported by the backend. >> > For PCI, implementing the post_plugged would be much more >> > complicated, so the current fix consists in checking whether >> > the backend supports VIRTIO_F_VERSION_1 in the backend. >> > >> > This patch propose to replace existing solutions with a common >> > approach by negociating features before ->device_plugged() is >> > called. A pre_plugged callback is introduced so that the >> > transports can set their supported features. >> > >> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >> > Cc: Marcel Apfelbaum <marcel@redhat.com> >> > Cc: Michael S. Tsirkin <mst@redhat.com> >> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Looks good - can you pls test and post a non-RFC? > Do you mean having this also in stable, or keeping it based on top of former pci-only patch? I already tested it for PCI, but cannot test it for CCW. Thanks, Maxime ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated 2016-09-09 16:16 ` Maxime Coquelin @ 2016-09-09 18:44 ` Michael S. Tsirkin 0 siblings, 0 replies; 20+ messages in thread From: Michael S. Tsirkin @ 2016-09-09 18:44 UTC (permalink / raw) To: Maxime Coquelin; +Cc: qemu-devel, cornelia.huck, marcel, vkaplans On Fri, Sep 09, 2016 at 06:16:27PM +0200, Maxime Coquelin wrote: > > > On 09/09/2016 05:39 PM, Michael S. Tsirkin wrote: > > On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote: > > > > Currently, devices are plugged before features are negotiated. > > > > If the backend doesn't support VIRTIO_F_VERSION_1, the transport > > > > need to rewind some settings. > > > > > > > > This is the case for both PCI and CCW. > > > > For CCW, a post_plugged callback had been introduced, where > > > > max_rev field is just updated if VIRTIO_F_VERSION_1 is not > > > > supported by the backend. > > > > For PCI, implementing the post_plugged would be much more > > > > complicated, so the current fix consists in checking whether > > > > the backend supports VIRTIO_F_VERSION_1 in the backend. > > > > > > > > This patch propose to replace existing solutions with a common > > > > approach by negociating features before ->device_plugged() is > > > > called. A pre_plugged callback is introduced so that the > > > > transports can set their supported features. > > > > > > > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > > > > Cc: Marcel Apfelbaum <marcel@redhat.com> > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > Looks good - can you pls test and post a non-RFC? > > > > Do you mean having this also in stable, or keeping > it based on top of former pci-only patch? > > I already tested it for PCI, but cannot test it for CCW. > > Thanks, > Maxime Once you repost, ask for help testing. -- MST ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-09-09 18:44 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-09 10:14 [Qemu-devel] [PATCH 0/2] virtio-pci: Improve device plugging whith legacy backends Maxime Coquelin 2016-09-09 10:14 ` [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality Maxime Coquelin 2016-09-09 10:33 ` Cornelia Huck 2016-09-09 10:48 ` Marcel Apfelbaum 2016-09-09 10:55 ` Cornelia Huck 2016-09-09 11:02 ` Marcel Apfelbaum 2016-09-09 11:20 ` Cornelia Huck 2016-09-09 11:36 ` Maxime Coquelin 2016-09-09 11:47 ` Marcel Apfelbaum 2016-09-09 10:14 ` [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 Maxime Coquelin 2016-09-09 10:40 ` Cornelia Huck 2016-09-09 11:04 ` Marcel Apfelbaum 2016-09-09 11:20 ` Cornelia Huck 2016-09-09 11:44 ` Maxime Coquelin 2016-09-09 11:49 ` Cornelia Huck 2016-09-09 12:01 ` Maxime Coquelin 2016-09-09 14:14 ` [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated Maxime Coquelin 2016-09-09 15:39 ` Michael S. Tsirkin 2016-09-09 16:16 ` Maxime Coquelin 2016-09-09 18:44 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.