| Message ID | 1467739394-28357-1-git-send-email-lersek@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jul 05, 2016 at 07:23:14PM +0200, Laszlo Ersek wrote: > At the moment the following QEMU command line triggers an assertion > failure (minimal reproducer by Cole): > > qemu-system-aarch64 \ > -machine virt-2.6,accel=tcg \ > -nodefaults \ > -no-user-config \ > -nographic -monitor stdio \ > -device virtio-scsi-device,id=scsi0 \ > -device virtio-scsi-device,id=scsi1 \ > -drive file=foo.img,format=raw,if=none,id=d0 \ > -device scsi-hd,bus=scsi0.0,drive=d0 \ > -drive file=foo.img,format=raw,if=none,id=d1 \ > -device scsi-hd,bus=scsi1.0,drive=d1 > > qemu-system-aarch64: migration/savevm.c:615: > vmstate_register_with_alias_id: > Assertion `!se->compat || se->instance_id == 0' failed. > > The reason is that the vmstate sections for the two scsi-hd devices are > not uniquely identifiable by name. > > The direct parent buses of the scsi-hd devices -- scsi0.0 and scsi1.0 -- > support the BusClass.get_dev_path member function. scsibus_get_dev_path() > formats a device path prefix with the help of its topologically parent > bus, and then appends the chan:id:lun triplet to it. For both scsi-hd > devices, this triplet is 0:0:0. > > (Here we use "device path" in the QEMU migration sense, for vmstate > section identification, not in the OFW or UEFI device path senses.) > > The virtio-scsi HBA is plugged into the virtio-mmio bus (implemented by > the internal VirtIOMMIOProxy device). This bus class > (TYPE_VIRTIO_MMIO_BUS) inherits, as its get_dev_path() member function, > the virtio_bus_get_dev_path() method from its parent class > (TYPE_VIRTIO_BUS). > > virtio_bus_get_dev_path() does not format any kind of device address on > its own; "virtio addresses" are transport-specific. Therefore > virtio_bus_get_dev_path() asks the topologically parent bus of the proxy > object (implementing the specific virtio transport) to format the address > of the proxy object. > > (For virtio-pci devices (where the proxy is an instance of VirtIOPCIProxy, > plugged into a PCI bus), this ends up in pcibus_get_dev_path().) > > However, VirtIOMMIOProxy is usually (in practice: always) plugged into > "main-system-bus", the singleton TYPE_SYSTEM_BUS object. This BusClass > does not support formatting QEMU vmstate device paths at all (as > SysBusDevice objects can have zero or more IO ports and zero or more MMIO > regions). Hence the formatting request delegated from > virtio_bus_get_dev_path() gets answered with NULL. > > The end result is that the two scsi-hd devices end up with the same device > path "0:0:0", which triggers the assert. > > We can solve this by recognizing that virtio-mmio transports are > distinguished from each other by their base addresses in MMIO address > space. Implement virtio_mmio_bus_get_dev_path() as follows: > > (1) The virtio device whose devpath is to be formatted resides on a > virtio-mmio bus that is implemented by a VirtIOMMIOProxy object. Ask > the parent bus of VirtIOMMIOProxy to format the device path of > VirtIOMMIOProxy, as a path prefix. (This is identical to what > virtio_bus_get_dev_path() does.) > > (2) Append the base address of VirtIOMMIOProxy to the device path, such > as: > - virtio-mmio@000000000a003e00, > - virtio-mmio@000000000a003c00. > > Given that these device paths are placed in the migration stream, step (2) > above, if done unconditionally, would break migration. So make that step > conditional on a new VirtIOMMIOProxy property, which is enabled for 2.7 > machine types and later. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Cole Robinson <crobinso@redhat.com> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Cc: Kevin Zhao <kevin.zhao@linaro.org> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Tom Hanson <thomas.hanson@linaro.org> > Reported-by: Kevin Zhao <kevin.zhao@linaro.org> > Fixes: https://bugs.launchpad.net/qemu/+bug/1594239 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > Actual migration testing with the patch would be appreciated ;) > > include/hw/compat.h | 6 +++++- > hw/virtio/virtio-mmio.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 636befedb497..9914e7a59ea1 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,11 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_6 \ > - /* empty */ > + {\ > + .driver = "virtio-mmio",\ > + .property = "format_transport_address",\ > + .value = "off",\ > + }, > > #define HW_COMPAT_2_5 \ > {\ > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c > index eb84b74532e9..13798b3cb844 100644 > --- a/hw/virtio/virtio-mmio.c > +++ b/hw/virtio/virtio-mmio.c > @@ -91,6 +91,7 @@ typedef struct { > VirtioBusState bus; > bool ioeventfd_disabled; > bool ioeventfd_started; > + bool format_transport_address; > } VirtIOMMIOProxy; > > static bool virtio_mmio_ioeventfd_started(DeviceState *d) > @@ -469,6 +470,12 @@ assign_error: > > /* virtio-mmio device */ > > +static Property virtio_mmio_properties[] = { > + DEFINE_PROP_BOOL("format_transport_address", VirtIOMMIOProxy, > + format_transport_address, true), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void virtio_mmio_realizefn(DeviceState *d, Error **errp) > { > VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d); > @@ -489,6 +496,7 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data) > dc->realize = virtio_mmio_realizefn; > dc->reset = virtio_mmio_reset; > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > + dc->props = virtio_mmio_properties; > } > > static const TypeInfo virtio_mmio_info = { > @@ -500,6 +508,46 @@ static const TypeInfo virtio_mmio_info = { > > /* virtio-mmio-bus. */ > > +static char *virtio_mmio_bus_get_dev_path(DeviceState *dev) > +{ > + BusState *virtio_mmio_bus; > + VirtIOMMIOProxy *virtio_mmio_proxy; > + char *proxy_path; > + SysBusDevice *proxy_sbd; > + char *path; > + > + virtio_mmio_bus = qdev_get_parent_bus(dev); > + virtio_mmio_proxy = VIRTIO_MMIO(virtio_mmio_bus->parent); > + proxy_path = qdev_get_dev_path(DEVICE(virtio_mmio_proxy)); > + > + /* > + * If @format_transport_address is false, then we just perform the same as > + * virtio_bus_get_dev_path(): we delegate the address formatting for the > + * device on the virtio-mmio bus to the bus that the virtio-mmio proxy > + * (i.e., the device that implements the virtio-mmio bus) resides on. In > + * this case the base address of the virtio-mmio transport will be > + * invisible. > + */ > + if (!virtio_mmio_proxy->format_transport_address) { > + return proxy_path; > + } > + > + /* Otherwise, we append the base address of the transport. */ > + proxy_sbd = SYS_BUS_DEVICE(virtio_mmio_proxy); > + assert(proxy_sbd->num_mmio == 1); > + assert(proxy_sbd->mmio[0].memory == &virtio_mmio_proxy->iomem); > + > + if (proxy_path) { > + path = g_strdup_printf("%s/virtio-mmio@" TARGET_FMT_plx, proxy_path, > + proxy_sbd->mmio[0].addr); > + } else { > + path = g_strdup_printf("virtio-mmio@" TARGET_FMT_plx, > + proxy_sbd->mmio[0].addr); > + } > + g_free(proxy_path); > + return path; > +} > + > static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data) > { > BusClass *bus_class = BUS_CLASS(klass); > @@ -516,6 +564,7 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data) > k->ioeventfd_assign = virtio_mmio_ioeventfd_assign; > k->has_variable_vring_alignment = true; > bus_class->max_dev = 1; > + bus_class->get_dev_path = virtio_mmio_bus_get_dev_path; > } > > static const TypeInfo virtio_mmio_bus_info = { > -- > 1.8.3.1 > > FWIW, this patch looks good to me. Reviewed-by: Andrew Jones <drjones@redhat.com>
On 7 July 2016 at 17:15, Andrew Jones <drjones@redhat.com> wrote: > On Tue, Jul 05, 2016 at 07:23:14PM +0200, Laszlo Ersek wrote: >> At the moment the following QEMU command line triggers an assertion >> failure (minimal reproducer by Cole): >> >> qemu-system-aarch64 \ >> -machine virt-2.6,accel=tcg \ >> -nodefaults \ >> -no-user-config \ >> -nographic -monitor stdio \ >> -device virtio-scsi-device,id=scsi0 \ >> -device virtio-scsi-device,id=scsi1 \ >> -drive file=foo.img,format=raw,if=none,id=d0 \ >> -device scsi-hd,bus=scsi0.0,drive=d0 \ >> -drive file=foo.img,format=raw,if=none,id=d1 \ >> -device scsi-hd,bus=scsi1.0,drive=d1 >> >> qemu-system-aarch64: migration/savevm.c:615: >> vmstate_register_with_alias_id: >> Assertion `!se->compat || se->instance_id == 0' failed. >> >> The reason is that the vmstate sections for the two scsi-hd devices are >> not uniquely identifiable by name. >> >> The direct parent buses of the scsi-hd devices -- scsi0.0 and scsi1.0 -- >> support the BusClass.get_dev_path member function. scsibus_get_dev_path() >> formats a device path prefix with the help of its topologically parent >> bus, and then appends the chan:id:lun triplet to it. For both scsi-hd >> devices, this triplet is 0:0:0. >> >> (Here we use "device path" in the QEMU migration sense, for vmstate >> section identification, not in the OFW or UEFI device path senses.) >> >> The virtio-scsi HBA is plugged into the virtio-mmio bus (implemented by >> the internal VirtIOMMIOProxy device). This bus class >> (TYPE_VIRTIO_MMIO_BUS) inherits, as its get_dev_path() member function, >> the virtio_bus_get_dev_path() method from its parent class >> (TYPE_VIRTIO_BUS). >> >> virtio_bus_get_dev_path() does not format any kind of device address on >> its own; "virtio addresses" are transport-specific. Therefore >> virtio_bus_get_dev_path() asks the topologically parent bus of the proxy >> object (implementing the specific virtio transport) to format the address >> of the proxy object. >> >> (For virtio-pci devices (where the proxy is an instance of VirtIOPCIProxy, >> plugged into a PCI bus), this ends up in pcibus_get_dev_path().) >> >> However, VirtIOMMIOProxy is usually (in practice: always) plugged into >> "main-system-bus", the singleton TYPE_SYSTEM_BUS object. This BusClass >> does not support formatting QEMU vmstate device paths at all (as >> SysBusDevice objects can have zero or more IO ports and zero or more MMIO >> regions). Hence the formatting request delegated from >> virtio_bus_get_dev_path() gets answered with NULL. >> >> The end result is that the two scsi-hd devices end up with the same device >> path "0:0:0", which triggers the assert. >> >> We can solve this by recognizing that virtio-mmio transports are >> distinguished from each other by their base addresses in MMIO address >> space. Implement virtio_mmio_bus_get_dev_path() as follows: >> >> (1) The virtio device whose devpath is to be formatted resides on a >> virtio-mmio bus that is implemented by a VirtIOMMIOProxy object. Ask >> the parent bus of VirtIOMMIOProxy to format the device path of >> VirtIOMMIOProxy, as a path prefix. (This is identical to what >> virtio_bus_get_dev_path() does.) >> >> (2) Append the base address of VirtIOMMIOProxy to the device path, such >> as: >> - virtio-mmio@000000000a003e00, >> - virtio-mmio@000000000a003c00. >> >> Given that these device paths are placed in the migration stream, step (2) >> above, if done unconditionally, would break migration. So make that step >> conditional on a new VirtIOMMIOProxy property, which is enabled for 2.7 >> machine types and later. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Cole Robinson <crobinso@redhat.com> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >> Cc: Kevin Zhao <kevin.zhao@linaro.org> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Cc: Tom Hanson <thomas.hanson@linaro.org> >> Reported-by: Kevin Zhao <kevin.zhao@linaro.org> >> Fixes: https://bugs.launchpad.net/qemu/+bug/1594239 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > FWIW, this patch looks good to me. > > Reviewed-by: Andrew Jones <drjones@redhat.com> Thanks; applied to target-arm.next. -- PMM
diff --git a/include/hw/compat.h b/include/hw/compat.h index 636befedb497..9914e7a59ea1 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -2,7 +2,11 @@ #define HW_COMPAT_H #define HW_COMPAT_2_6 \ - /* empty */ + {\ + .driver = "virtio-mmio",\ + .property = "format_transport_address",\ + .value = "off",\ + }, #define HW_COMPAT_2_5 \ {\ diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index eb84b74532e9..13798b3cb844 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -91,6 +91,7 @@ typedef struct { VirtioBusState bus; bool ioeventfd_disabled; bool ioeventfd_started; + bool format_transport_address; } VirtIOMMIOProxy; static bool virtio_mmio_ioeventfd_started(DeviceState *d) @@ -469,6 +470,12 @@ assign_error: /* virtio-mmio device */ +static Property virtio_mmio_properties[] = { + DEFINE_PROP_BOOL("format_transport_address", VirtIOMMIOProxy, + format_transport_address, true), + DEFINE_PROP_END_OF_LIST(), +}; + static void virtio_mmio_realizefn(DeviceState *d, Error **errp) { VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d); @@ -489,6 +496,7 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data) dc->realize = virtio_mmio_realizefn; dc->reset = virtio_mmio_reset; set_bit(DEVICE_CATEGORY_MISC, dc->categories); + dc->props = virtio_mmio_properties; } static const TypeInfo virtio_mmio_info = { @@ -500,6 +508,46 @@ static const TypeInfo virtio_mmio_info = { /* virtio-mmio-bus. */ +static char *virtio_mmio_bus_get_dev_path(DeviceState *dev) +{ + BusState *virtio_mmio_bus; + VirtIOMMIOProxy *virtio_mmio_proxy; + char *proxy_path; + SysBusDevice *proxy_sbd; + char *path; + + virtio_mmio_bus = qdev_get_parent_bus(dev); + virtio_mmio_proxy = VIRTIO_MMIO(virtio_mmio_bus->parent); + proxy_path = qdev_get_dev_path(DEVICE(virtio_mmio_proxy)); + + /* + * If @format_transport_address is false, then we just perform the same as + * virtio_bus_get_dev_path(): we delegate the address formatting for the + * device on the virtio-mmio bus to the bus that the virtio-mmio proxy + * (i.e., the device that implements the virtio-mmio bus) resides on. In + * this case the base address of the virtio-mmio transport will be + * invisible. + */ + if (!virtio_mmio_proxy->format_transport_address) { + return proxy_path; + } + + /* Otherwise, we append the base address of the transport. */ + proxy_sbd = SYS_BUS_DEVICE(virtio_mmio_proxy); + assert(proxy_sbd->num_mmio == 1); + assert(proxy_sbd->mmio[0].memory == &virtio_mmio_proxy->iomem); + + if (proxy_path) { + path = g_strdup_printf("%s/virtio-mmio@" TARGET_FMT_plx, proxy_path, + proxy_sbd->mmio[0].addr); + } else { + path = g_strdup_printf("virtio-mmio@" TARGET_FMT_plx, + proxy_sbd->mmio[0].addr); + } + g_free(proxy_path); + return path; +} + static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data) { BusClass *bus_class = BUS_CLASS(klass); @@ -516,6 +564,7 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data) k->ioeventfd_assign = virtio_mmio_ioeventfd_assign; k->has_variable_vring_alignment = true; bus_class->max_dev = 1; + bus_class->get_dev_path = virtio_mmio_bus_get_dev_path; } static const TypeInfo virtio_mmio_bus_info = {
At the moment the following QEMU command line triggers an assertion failure (minimal reproducer by Cole): qemu-system-aarch64 \ -machine virt-2.6,accel=tcg \ -nodefaults \ -no-user-config \ -nographic -monitor stdio \ -device virtio-scsi-device,id=scsi0 \ -device virtio-scsi-device,id=scsi1 \ -drive file=foo.img,format=raw,if=none,id=d0 \ -device scsi-hd,bus=scsi0.0,drive=d0 \ -drive file=foo.img,format=raw,if=none,id=d1 \ -device scsi-hd,bus=scsi1.0,drive=d1 qemu-system-aarch64: migration/savevm.c:615: vmstate_register_with_alias_id: Assertion `!se->compat || se->instance_id == 0' failed. The reason is that the vmstate sections for the two scsi-hd devices are not uniquely identifiable by name. The direct parent buses of the scsi-hd devices -- scsi0.0 and scsi1.0 -- support the BusClass.get_dev_path member function. scsibus_get_dev_path() formats a device path prefix with the help of its topologically parent bus, and then appends the chan:id:lun triplet to it. For both scsi-hd devices, this triplet is 0:0:0. (Here we use "device path" in the QEMU migration sense, for vmstate section identification, not in the OFW or UEFI device path senses.) The virtio-scsi HBA is plugged into the virtio-mmio bus (implemented by the internal VirtIOMMIOProxy device). This bus class (TYPE_VIRTIO_MMIO_BUS) inherits, as its get_dev_path() member function, the virtio_bus_get_dev_path() method from its parent class (TYPE_VIRTIO_BUS). virtio_bus_get_dev_path() does not format any kind of device address on its own; "virtio addresses" are transport-specific. Therefore virtio_bus_get_dev_path() asks the topologically parent bus of the proxy object (implementing the specific virtio transport) to format the address of the proxy object. (For virtio-pci devices (where the proxy is an instance of VirtIOPCIProxy, plugged into a PCI bus), this ends up in pcibus_get_dev_path().) However, VirtIOMMIOProxy is usually (in practice: always) plugged into "main-system-bus", the singleton TYPE_SYSTEM_BUS object. This BusClass does not support formatting QEMU vmstate device paths at all (as SysBusDevice objects can have zero or more IO ports and zero or more MMIO regions). Hence the formatting request delegated from virtio_bus_get_dev_path() gets answered with NULL. The end result is that the two scsi-hd devices end up with the same device path "0:0:0", which triggers the assert. We can solve this by recognizing that virtio-mmio transports are distinguished from each other by their base addresses in MMIO address space. Implement virtio_mmio_bus_get_dev_path() as follows: (1) The virtio device whose devpath is to be formatted resides on a virtio-mmio bus that is implemented by a VirtIOMMIOProxy object. Ask the parent bus of VirtIOMMIOProxy to format the device path of VirtIOMMIOProxy, as a path prefix. (This is identical to what virtio_bus_get_dev_path() does.) (2) Append the base address of VirtIOMMIOProxy to the device path, such as: - virtio-mmio@000000000a003e00, - virtio-mmio@000000000a003c00. Given that these device paths are placed in the migration stream, step (2) above, if done unconditionally, would break migration. So make that step conditional on a new VirtIOMMIOProxy property, which is enabled for 2.7 machine types and later. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Cole Robinson <crobinso@redhat.com> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> Cc: Kevin Zhao <kevin.zhao@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Tom Hanson <thomas.hanson@linaro.org> Reported-by: Kevin Zhao <kevin.zhao@linaro.org> Fixes: https://bugs.launchpad.net/qemu/+bug/1594239 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Actual migration testing with the patch would be appreciated ;) include/hw/compat.h | 6 +++++- hw/virtio/virtio-mmio.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) -- 1.8.3.1