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

[Michael S. Tsirkin]

On Mon, Oct 13, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
> 
> 
> On 13.10.14 12:42, Greg Kurz wrote:
> > On Mon, 13 Oct 2014 12:01:07 +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). 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
> >>
> >> AFAIK pseries doesn't support cross-version migration, does it?
> 
> We're trying to make sure we don't break cross-version migration for the
> pseries machines. If nothing else, at least as a learning exercise.
> 
> 
> Alex

I see.
In that case, I think we need a common header, where
we can add device compatibility defines.
With PC and PSERIES would then pull it in.

Let's not duplicate code.

I'll try to come up with a patch.

-- 
MST