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

[Greg Kurz]

On Mon, 13 Oct 2014 14:09:54 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Mon, Oct 13, 2014 at 10:49:41AM +0200, Greg Kurz wrote:
> > 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).
> 
> This will reproduce with rhel5 though.
> 
> > I see bus
> > master is set early (bios ?) and never unset...
> 
> IIUC if you don't boot from device, it won't be set,
> and will not be set with older guests.
> 

Not sure to follow... you mean that if I boot from memory ? Like,
passing -kernel and friends to QEMU ?

> > 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(),
> > >  };
> > > 
>