Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master

[Greg Kurz]

On Mon, 6 Oct 2014 20:25:04 +0300
"Michael S. Tsirkin" <address@hidden> wrote:
> [...]
> 
> BTW I reverted that patch, and to fix migration, I'm thinking
> about applying the following patch on top of master.
> 

Michael,

I could force the migration issue with a rhel65 guest thanks to the
following patch, applied to hw/virtio/virtio-pci.c in QEMU v2.1.

@@ -271,6 +272,16 @@ static void virtio_ioport_write(void *opaque, uint32_t 
addr, uint32_t val)
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
     hwaddr pa;
 
+    if ((vdev->status == (VIRTIO_CONFIG_S_ACKNOWLEDGE | 
VIRTIO_CONFIG_S_DRIVER))
+        && (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER))
+        && getenv("MIG_BUG"))
+    {
+        fprintf(stderr, "\n\n\n\tMIGRATE !\n\n\n");
+        qmp_migrate(getenv("MIG_BUG"), false, false, false, false, false,
+                        false, NULL);
+        unsetenv("MIG_BUG");
+    }
+
     switch (addr) {
     case VIRTIO_PCI_GUEST_FEATURES:
         /* Guest does not negotiate properly?  We have to assume nothing. */


Indeed, the destination QEMU master hangs because bus master isn't set.

> Would appreciate testing cross-version migration (2.1 to master)
> with this patch applied.
> 

I had first to to enable the property for pseries of course. I could then
migrate QEMU v2.1 to QEMU master and back to QEMU v2.1, in the window
where we have DRIVER enabled and MASTER disabled, without experiencing
the hang.

Your fix works as expected (just a remark below).

> 
> 
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 1cea157..8873b6d 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -53,6 +53,11 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>  #define VIRTIO_PCI_BUS_CLASS(klass) \
>          OBJECT_CLASS_CHECK(VirtioPCIBusClass, klass, TYPE_VIRTIO_PCI_BUS)
> 
> +/* Need to activate work-arounds for buggy guests at vmstate load. */
> +#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT  0
> +#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION \
> +    (1 << VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT)
> +
>  /* Performance improves when virtqueue kick processing is decoupled from the
>   * vcpu thread using ioeventfd for some devices. */
>  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index bae023a..e07b6c4 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -312,6 +312,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t 
> *);
>              .driver   = "intel-hda",\
>              .property = "old_msi_addr",\
>              .value    = "on",\
> +        },\
> +        {\
> +            .driver   = "virtio-pci",\
> +            .property = "virtio-pci-bus-master-bug-migration",\
> +            .value    = "on",\
>          }
> 
>  #define PC_COMPAT_2_0 \

FWIW, the issue does not occur with intel targets, at least not
in my test case (booting rhel6 on a virtio-blk disk). I see bus
master is set early (bios ?) and never unset...

If you decide to apply for intel anyway, shouldn't the enablement be
in a separate patch ?

Will you resend or would you like me to do it, along with the pseries
enablement part ? In this case, I would need your SoB for the present
patch.

Thanks.

--
Greg

> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index a827cd4..a499a3c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -86,9 +86,6 @@
>   * 12 is historical, and due to x86 page size. */
>  #define VIRTIO_PCI_QUEUE_ADDR_SHIFT    12
> 
> -/* Flags track per-device state like workarounds for quirks in older guests. 
> */
> -#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
> -
>  static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
>                                 VirtIOPCIProxy *dev);
> 
> @@ -323,14 +320,6 @@ static void virtio_ioport_write(void *opaque, uint32_t 
> addr, uint32_t val)
>                                       proxy->pci_dev.config[PCI_COMMAND] |
>                                       PCI_COMMAND_MASTER, 1);
>          }
> -
> -        /* Linux before 2.6.34 sets the device as OK without enabling
> -           the PCI device bus master bit. In this case we need to disable
> -           some safety checks. */
> -        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> -            !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> -            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> -        }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
>          msix_vector_unuse(&proxy->pci_dev, vdev->config_vector);
> @@ -483,8 +472,7 @@ static void virtio_write_config(PCIDevice *pci_dev, 
> uint32_t address,
>      pci_default_write_config(pci_dev, address, val, len);
> 
>      if (range_covers_byte(address, len, PCI_COMMAND) &&
> -        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
> -        !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) {
> +        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>          virtio_pci_stop_ioeventfd(proxy);
>          virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
>      }
> @@ -895,11 +883,15 @@ static void virtio_pci_vmstate_change(DeviceState *d, 
> bool running)
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> 
>      if (running) {
> -        /* Try to find out if the guest has bus master disabled, but is
> -           in ready state. Then we have a buggy guest OS. */
> -        if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +        /* Old QEMU versions did not set bus master enable on status write.
> +         * Detect DRIVER set and enable it.
> +         */
> +        if ((proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION) &&
> +            (vdev->status & VIRTIO_CONFIG_S_DRIVER) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> -            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            pci_default_write_config(&proxy->pci_dev, PCI_COMMAND,
> +                                     proxy->pci_dev.config[PCI_COMMAND] |
> +                                     PCI_COMMAND_MASTER, 1);
>          }
>          virtio_pci_start_ioeventfd(proxy);
>      } else {
> @@ -1040,10 +1032,11 @@ static void virtio_pci_reset(DeviceState *qdev)
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_bus_reset(bus);
>      msix_unuse_all_vectors(&proxy->pci_dev);
> -    proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>  }
> 
>  static Property virtio_pci_properties[] = {
> +    DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, 
> flags,
> +                    VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>