Message ID | 1449065398-45267-1-git-send-email-cornelia.huck@de.ibm.com |
---|---|
State | New |
Headers | show |
On Wed, Dec 02, 2015 at 03:09:58PM +0100, Cornelia Huck wrote: > If you run a qemu advertising VERSION_1 with an old kernel where > vhost did not yet support VERSION_1, you'll end up with a device > that is {modern pci|ccw revision 1} but does not advertise VERSION_1. > This is not a sensible configuration and is rejected by the Linux > guest drivers. > > To fix this, add a ->post_plugged() callback invoked after features > have been queried that can handle the VERSION_1 bit being withdrawn > and change pci (only setup modern if VERSION_1 is still present) and > ccw (fall back to revision 0 if VERSION_1 is gone). > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> Unfortunately this is too late: we need to know whether modern will be supported to know whether to add the pci express capability :( Maybe this should be moved into plugged callback? > --- > Michael: I think this should go into 2.5, as otherwise ccw (which > defaults to virtio-1 with 2.5) will be broken with old host kernels. > I need someone to give virtio-pci some testing, as I do not have > a proper setup for that. ccw is working fine. > > Changes from initial writeup: > - Move pci-modern init to post_plugged instead of undoing [Jason] > - Add a proper patch description + signoff > - Add some comments > --- > hw/s390x/virtio-ccw.c | 12 ++++++ > hw/virtio/virtio-bus.c | 3 ++ > hw/virtio/virtio-pci.c | 96 ++++++++++++++++++++++++++---------------- > include/hw/virtio/virtio-bus.h | 5 +++ > 4 files changed, 80 insertions(+), 36 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index fb103b7..63da303 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1555,6 +1555,17 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) > 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); > @@ -1891,6 +1902,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data) > k->save_config = virtio_ccw_save_config; > k->load_config = virtio_ccw_load_config; > k->device_plugged = virtio_ccw_device_plugged; > + k->post_plugged = virtio_ccw_post_plugged; > k->device_unplugged = virtio_ccw_device_unplugged; > } > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index febda76..81c7cdd 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -56,6 +56,9 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > 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); > + } > } > > /* Reset the virtio_bus */ > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index dd48562..af59496 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1626,7 +1626,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > VirtioBusState *bus = &proxy->bus; > bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); > bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); > - bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; > uint8_t *config; > uint32_t size; > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > @@ -1653,6 +1652,65 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > > > if (modern) { > + virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); > + /* Let's see if this feature bit sticks before doing further init. */ > + } > + > + if (proxy->nvectors) { > + int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, > + proxy->msix_bar); > + if (err) { > + /* Notice when a system that supports MSIx can't initialize it. */ > + if (err != -ENOTSUP) { > + error_report("unable to init msix vectors to %" PRIu32, > + proxy->nvectors); > + } > + proxy->nvectors = 0; > + } > + } > + > + proxy->pci_dev.config_write = virtio_write_config; > + proxy->pci_dev.config_read = virtio_read_config; > + > + if (legacy) { > + size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) > + + virtio_bus_get_vdev_config_len(bus); > + size = pow2ceil(size); > + > + memory_region_init_io(&proxy->bar, OBJECT(proxy), > + &virtio_pci_config_ops, > + proxy, "virtio-pci", size); > + > + pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar, > + PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar); > + } > + > + 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_post_plugged(DeviceState *d, Error **errp) > +{ > + VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > + bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); > + bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); > + bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > + > + if (modern && !virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + /* A backend didn't support modern virtio. */ > + if (legacy) { > + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; > + modern = false; > + } else { > + error_setg(errp, "can't fall back to legacy virtio"); > + return; > + } > + } > + if (modern) { > struct virtio_pci_cap cap = { > .cap_len = sizeof cap, > }; > @@ -1672,7 +1730,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); > @@ -1705,40 +1762,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > pci_set_long(cfg_mask->pci_cfg_data, ~0x0); > } > > - if (proxy->nvectors) { > - int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, > - proxy->msix_bar); > - if (err) { > - /* Notice when a system that supports MSIx can't initialize it. */ > - if (err != -ENOTSUP) { > - error_report("unable to init msix vectors to %" PRIu32, > - proxy->nvectors); > - } > - proxy->nvectors = 0; > - } > - } > - > - proxy->pci_dev.config_write = virtio_write_config; > - proxy->pci_dev.config_read = virtio_read_config; > - > - if (legacy) { > - size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) > - + virtio_bus_get_vdev_config_len(bus); > - size = pow2ceil(size); > - > - memory_region_init_io(&proxy->bar, OBJECT(proxy), > - &virtio_pci_config_ops, > - proxy, "virtio-pci", size); > - > - pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar, > - PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar); > - } > - > - 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) > @@ -2474,6 +2497,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) > k->set_guest_notifiers = virtio_pci_set_guest_notifiers; > k->vmstate_change = virtio_pci_vmstate_change; > k->device_plugged = virtio_pci_device_plugged; > + k->post_plugged = virtio_pci_post_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 6c3d4cb..3f2c136 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -60,6 +60,11 @@ typedef struct VirtioBusClass { > */ > 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.3.9
On Wed, 2 Dec 2015 18:43:05 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2015 at 03:09:58PM +0100, Cornelia Huck wrote: > > If you run a qemu advertising VERSION_1 with an old kernel where > > vhost did not yet support VERSION_1, you'll end up with a device > > that is {modern pci|ccw revision 1} but does not advertise VERSION_1. > > This is not a sensible configuration and is rejected by the Linux > > guest drivers. > > > > To fix this, add a ->post_plugged() callback invoked after features > > have been queried that can handle the VERSION_1 bit being withdrawn > > and change pci (only setup modern if VERSION_1 is still present) and > > ccw (fall back to revision 0 if VERSION_1 is gone). > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > Unfortunately this is too late: we need to know > whether modern will be supported to know whether > to add the pci express capability :( > > Maybe this should be moved into plugged > callback? As discussed on irc: I'll cut this down to ccw only for 2.5, and we'll rethink for 2.6. Patch in progress.
On 12/02/2015 05:43 PM, Michael S. Tsirkin wrote: > On Wed, Dec 02, 2015 at 03:09:58PM +0100, Cornelia Huck wrote: >> If you run a qemu advertising VERSION_1 with an old kernel where >> vhost did not yet support VERSION_1, you'll end up with a device >> that is {modern pci|ccw revision 1} but does not advertise VERSION_1. >> This is not a sensible configuration and is rejected by the Linux >> guest drivers. >> >> To fix this, add a ->post_plugged() callback invoked after features >> have been queried that can handle the VERSION_1 bit being withdrawn >> and change pci (only setup modern if VERSION_1 is still present) and >> ccw (fall back to revision 0 if VERSION_1 is gone). >> >> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > Unfortunately this is too late: we need to know > whether modern will be supported to know whether > to add the pci express capability :( I can at least confirm that this patch "fixes" the guest error message virtio_net virtio0: virtio: device uses modern interface but does not have VIRTIO_F_VERSION_1 virtio_net: probe of virtio0 failed with error -22 is gone. but yes, its not enough for the other aspects. > > Maybe this should be moved into plugged > callback?
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index fb103b7..63da303 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1555,6 +1555,17 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp) 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); @@ -1891,6 +1902,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data) k->save_config = virtio_ccw_save_config; k->load_config = virtio_ccw_load_config; k->device_plugged = virtio_ccw_device_plugged; + k->post_plugged = virtio_ccw_post_plugged; k->device_unplugged = virtio_ccw_device_unplugged; } diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index febda76..81c7cdd 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -56,6 +56,9 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) 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); + } } /* Reset the virtio_bus */ diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index dd48562..af59496 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1626,7 +1626,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) VirtioBusState *bus = &proxy->bus; bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); - bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; uint8_t *config; uint32_t size; VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); @@ -1653,6 +1652,65 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) if (modern) { + virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1); + /* Let's see if this feature bit sticks before doing further init. */ + } + + if (proxy->nvectors) { + int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, + proxy->msix_bar); + if (err) { + /* Notice when a system that supports MSIx can't initialize it. */ + if (err != -ENOTSUP) { + error_report("unable to init msix vectors to %" PRIu32, + proxy->nvectors); + } + proxy->nvectors = 0; + } + } + + proxy->pci_dev.config_write = virtio_write_config; + proxy->pci_dev.config_read = virtio_read_config; + + if (legacy) { + size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + + virtio_bus_get_vdev_config_len(bus); + size = pow2ceil(size); + + memory_region_init_io(&proxy->bar, OBJECT(proxy), + &virtio_pci_config_ops, + proxy, "virtio-pci", size); + + pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar, + PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar); + } + + 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_post_plugged(DeviceState *d, Error **errp) +{ + VirtIOPCIProxy *proxy = VIRTIO_PCI(d); + bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); + bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); + bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + + if (modern && !virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) { + /* A backend didn't support modern virtio. */ + if (legacy) { + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; + modern = false; + } else { + error_setg(errp, "can't fall back to legacy virtio"); + return; + } + } + if (modern) { struct virtio_pci_cap cap = { .cap_len = sizeof cap, }; @@ -1672,7 +1730,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); @@ -1705,40 +1762,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) pci_set_long(cfg_mask->pci_cfg_data, ~0x0); } - if (proxy->nvectors) { - int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, - proxy->msix_bar); - if (err) { - /* Notice when a system that supports MSIx can't initialize it. */ - if (err != -ENOTSUP) { - error_report("unable to init msix vectors to %" PRIu32, - proxy->nvectors); - } - proxy->nvectors = 0; - } - } - - proxy->pci_dev.config_write = virtio_write_config; - proxy->pci_dev.config_read = virtio_read_config; - - if (legacy) { - size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) - + virtio_bus_get_vdev_config_len(bus); - size = pow2ceil(size); - - memory_region_init_io(&proxy->bar, OBJECT(proxy), - &virtio_pci_config_ops, - proxy, "virtio-pci", size); - - pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar, - PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar); - } - - 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) @@ -2474,6 +2497,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) k->set_guest_notifiers = virtio_pci_set_guest_notifiers; k->vmstate_change = virtio_pci_vmstate_change; k->device_plugged = virtio_pci_device_plugged; + k->post_plugged = virtio_pci_post_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 6c3d4cb..3f2c136 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -60,6 +60,11 @@ typedef struct VirtioBusClass { */ 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. */
If you run a qemu advertising VERSION_1 with an old kernel where vhost did not yet support VERSION_1, you'll end up with a device that is {modern pci|ccw revision 1} but does not advertise VERSION_1. This is not a sensible configuration and is rejected by the Linux guest drivers. To fix this, add a ->post_plugged() callback invoked after features have been queried that can handle the VERSION_1 bit being withdrawn and change pci (only setup modern if VERSION_1 is still present) and ccw (fall back to revision 0 if VERSION_1 is gone). Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- Michael: I think this should go into 2.5, as otherwise ccw (which defaults to virtio-1 with 2.5) will be broken with old host kernels. I need someone to give virtio-pci some testing, as I do not have a proper setup for that. ccw is working fine. Changes from initial writeup: - Move pci-modern init to post_plugged instead of undoing [Jason] - Add a proper patch description + signoff - Add some comments --- hw/s390x/virtio-ccw.c | 12 ++++++ hw/virtio/virtio-bus.c | 3 ++ hw/virtio/virtio-pci.c | 96 ++++++++++++++++++++++++++---------------- include/hw/virtio/virtio-bus.h | 5 +++ 4 files changed, 80 insertions(+), 36 deletions(-)