* [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master @ 2014-09-14 18:30 Michael S. Tsirkin 2014-09-17 17:21 ` Greg Kurz 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2014-09-14 18:30 UTC (permalink / raw) To: qemu-devel; +Cc: Jason Wang, Anthony Liguori Current support for bus master (clearing OK bit) together with the need to support guests which do not enable PCI bus mastering, leads to extra state in VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust in case of cross-version migration for the case when guests use the device before setting DRIVER_OK. Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler work-around: treat clearing of PCI_COMMAND as a virtio reset. Old guests never touch this bit so they will work. As reset clears device status, DRIVER and MASTER bits are now in sync, so we can fix up cross-version migration simply by synchronising them, without need to detect a buggy guest explicitly. Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. As reset makes the device quiescent, in the future we'll be able to drop checking OK bit in a bunch of places. Cc: Jason Wang <jasowang@redhat.com> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index a827cd4..f560814 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); @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; + 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)) { + (cmd & PCI_COMMAND_MASTER)) { + /* Bus driver disables bus mastering - make it act + * as a kind of reset to render the device quiescent. */ virtio_pci_stop_ioeventfd(proxy); - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); + virtio_reset(vdev); + msix_unuse_all_vectors(&proxy->pci_dev); } } @@ -895,11 +889,19 @@ 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) && - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; + /* Linux before 2.6.34 drives the device without enabling + the PCI device bus master bit. Enable it automatically + for the guest. This is a PCI spec violation but so is + initiating DMA with bus master bit clear. + Note: this only makes a difference when migrating + across QEMU versions from an old QEMU, as for new QEMU + bus master and driver bits are always in sync. + TODO: consider enabling conditionally for compat machine types. */ + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER)) { + 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,7 +1042,6 @@ 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[] = { -- MST ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-09-14 18:30 [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master Michael S. Tsirkin @ 2014-09-17 17:21 ` Greg Kurz 2014-09-17 17:39 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2014-09-17 17:21 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Jason Wang, qemu-devel, Anthony Liguori On Sun, 14 Sep 2014 21:30:36 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Current support for bus master (clearing OK bit) > together with the need to support guests which do not > enable PCI bus mastering, leads to extra state in > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > in case of cross-version migration for the case when > guests use the device before setting DRIVER_OK. > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > guests never touch this bit so they will work. > > As reset clears device status, DRIVER and MASTER bits are > now in sync, so we can fix up cross-version migration simply > by synchronising them, without need to detect a buggy guest > explicitly. > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > As reset makes the device quiescent, in the future we'll be able to drop > checking OK bit in a bunch of places. > > Cc: Jason Wang <jasowang@redhat.com> > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- Hi Michael, I am not quite sure how to test this patch with my pseries based setup... Migrating from qemu-2.1 to qemu-master ? Cheers. -- Greg > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 19 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index a827cd4..f560814 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); > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > + > 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)) { > + (cmd & PCI_COMMAND_MASTER)) { > + /* Bus driver disables bus mastering - make it act > + * as a kind of reset to render the device quiescent. */ > virtio_pci_stop_ioeventfd(proxy); > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > + virtio_reset(vdev); > + msix_unuse_all_vectors(&proxy->pci_dev); > } > } > > @@ -895,11 +889,19 @@ 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) && > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > + /* Linux before 2.6.34 drives the device without enabling > + the PCI device bus master bit. Enable it automatically > + for the guest. This is a PCI spec violation but so is > + initiating DMA with bus master bit clear. > + Note: this only makes a difference when migrating > + across QEMU versions from an old QEMU, as for new QEMU > + bus master and driver bits are always in sync. > + TODO: consider enabling conditionally for compat machine types. */ > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > + VIRTIO_CONFIG_S_DRIVER)) { > + 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,7 +1042,6 @@ 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[] = { ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-09-17 17:21 ` Greg Kurz @ 2014-09-17 17:39 ` Michael S. Tsirkin 2014-10-06 14:51 ` Greg Kurz 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2014-09-17 17:39 UTC (permalink / raw) To: Greg Kurz; +Cc: Jason Wang, qemu-devel, Anthony Liguori On Wed, Sep 17, 2014 at 07:21:09PM +0200, Greg Kurz wrote: > On Sun, 14 Sep 2014 21:30:36 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > Current support for bus master (clearing OK bit) > > together with the need to support guests which do not > > enable PCI bus mastering, leads to extra state in > > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > > in case of cross-version migration for the case when > > guests use the device before setting DRIVER_OK. > > > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > > guests never touch this bit so they will work. > > > > As reset clears device status, DRIVER and MASTER bits are > > now in sync, so we can fix up cross-version migration simply > > by synchronising them, without need to detect a buggy guest > > explicitly. > > > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > > > As reset makes the device quiescent, in the future we'll be able to drop > > checking OK bit in a bunch of places. > > > > Cc: Jason Wang <jasowang@redhat.com> > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Hi Michael, > > I am not quite sure how to test this patch with my pseries based setup... > Migrating from qemu-2.1 to qemu-master ? > > Cheers. > > -- > Greg Exactly. And back! Pls don't forget to specify the 2.1 machine type. Thanks! > > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index a827cd4..f560814 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); > > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > > + > > 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)) { > > + (cmd & PCI_COMMAND_MASTER)) { > > + /* Bus driver disables bus mastering - make it act > > + * as a kind of reset to render the device quiescent. */ > > virtio_pci_stop_ioeventfd(proxy); > > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > > + virtio_reset(vdev); > > + msix_unuse_all_vectors(&proxy->pci_dev); > > } > > } > > > > @@ -895,11 +889,19 @@ 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) && > > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > + /* Linux before 2.6.34 drives the device without enabling > > + the PCI device bus master bit. Enable it automatically > > + for the guest. This is a PCI spec violation but so is > > + initiating DMA with bus master bit clear. > > + Note: this only makes a difference when migrating > > + across QEMU versions from an old QEMU, as for new QEMU > > + bus master and driver bits are always in sync. > > + TODO: consider enabling conditionally for compat machine types. */ > > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > > + VIRTIO_CONFIG_S_DRIVER)) { > > + 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,7 +1042,6 @@ 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[] = { ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-09-17 17:39 ` Michael S. Tsirkin @ 2014-10-06 14:51 ` Greg Kurz 2014-10-06 16:26 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2014-10-06 14:51 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On Wed, 17 Sep 2014 20:39:25 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Sep 17, 2014 at 07:21:09PM +0200, Greg Kurz wrote: > > On Sun, 14 Sep 2014 21:30:36 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > Current support for bus master (clearing OK bit) > > > together with the need to support guests which do not > > > enable PCI bus mastering, leads to extra state in > > > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > > > in case of cross-version migration for the case when > > > guests use the device before setting DRIVER_OK. > > > > > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > > > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > > > guests never touch this bit so they will work. > > > > > > As reset clears device status, DRIVER and MASTER bits are > > > now in sync, so we can fix up cross-version migration simply > > > by synchronising them, without need to detect a buggy guest > > > explicitly. > > > > > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > > > > > As reset makes the device quiescent, in the future we'll be able to drop > > > checking OK bit in a bunch of places. > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > > Hi Michael, > > > > I am not quite sure how to test this patch with my pseries based setup... > > Migrating from qemu-2.1 to qemu-master ? > > > > Cheers. > > > > -- > > Greg > > Exactly. And back! Pls don't forget to specify the 2.1 machine type. > Thanks! > Michael, Nikunj and I had started to investigate the pseries breakage: the QEMU originated reset brought by this patch clears the vq and breaks SLOF. This isn't a surprise since reset should always come from the driver, not the device. Since commit 45363e46aeebfc99753389649eac7c7fc22bfe52 has reverted this patch, QEMU works again for pseries and virtio. :) So back to the initial issue, I've tried to migrate a pseries-2.1 guest running rhel65, from QEMU v2.1.2 to QEMU master, back and forth, several times and it always succeeded... what symptom this patch was expected to fix ? Cheers. -- Greg > > > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > index a827cd4..f560814 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); > > > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > > > + > > > 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)) { > > > + (cmd & PCI_COMMAND_MASTER)) { > > > + /* Bus driver disables bus mastering - make it act > > > + * as a kind of reset to render the device quiescent. */ > > > virtio_pci_stop_ioeventfd(proxy); > > > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > > > + virtio_reset(vdev); > > > + msix_unuse_all_vectors(&proxy->pci_dev); > > > } > > > } > > > > > > @@ -895,11 +889,19 @@ 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) && > > > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > > > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > + /* Linux before 2.6.34 drives the device without enabling > > > + the PCI device bus master bit. Enable it automatically > > > + for the guest. This is a PCI spec violation but so is > > > + initiating DMA with bus master bit clear. > > > + Note: this only makes a difference when migrating > > > + across QEMU versions from an old QEMU, as for new QEMU > > > + bus master and driver bits are always in sync. > > > + TODO: consider enabling conditionally for compat machine types. */ > > > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > > > + VIRTIO_CONFIG_S_DRIVER)) { > > > + 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,7 +1042,6 @@ 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[] = { > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-06 14:51 ` Greg Kurz @ 2014-10-06 16:26 ` Michael S. Tsirkin 2014-10-06 16:46 ` Greg Kurz 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2014-10-06 16:26 UTC (permalink / raw) To: Greg Kurz; +Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On Mon, Oct 06, 2014 at 04:51:35PM +0200, Greg Kurz wrote: > On Wed, 17 Sep 2014 20:39:25 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Sep 17, 2014 at 07:21:09PM +0200, Greg Kurz wrote: > > > On Sun, 14 Sep 2014 21:30:36 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > Current support for bus master (clearing OK bit) > > > > together with the need to support guests which do not > > > > enable PCI bus mastering, leads to extra state in > > > > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > > > > in case of cross-version migration for the case when > > > > guests use the device before setting DRIVER_OK. > > > > > > > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > > > > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > > > > guests never touch this bit so they will work. > > > > > > > > As reset clears device status, DRIVER and MASTER bits are > > > > now in sync, so we can fix up cross-version migration simply > > > > by synchronising them, without need to detect a buggy guest > > > > explicitly. > > > > > > > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > > > > > > > As reset makes the device quiescent, in the future we'll be able to drop > > > > checking OK bit in a bunch of places. > > > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > > > Hi Michael, > > > > > > I am not quite sure how to test this patch with my pseries based setup... > > > Migrating from qemu-2.1 to qemu-master ? > > > > > > Cheers. > > > > > > -- > > > Greg > > > > Exactly. And back! Pls don't forget to specify the 2.1 machine type. > > Thanks! > > > > Michael, > > Nikunj and I had started to investigate the pseries breakage: the QEMU > originated reset brought by this patch clears the vq and breaks SLOF. > This isn't a surprise since reset should always come from the driver, > not the device. > > Since commit 45363e46aeebfc99753389649eac7c7fc22bfe52 has reverted this > patch, QEMU works again for pseries and virtio. :) > > So back to the initial issue, I've tried to migrate a pseries-2.1 guest running > rhel65, from QEMU v2.1.2 to QEMU master, back and forth, several times and it > always succeeded... what symptom this patch was expected to fix ? > > Cheers. > > -- > Greg > > > > > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > > > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > > index a827cd4..f560814 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); > > > > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > > > > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > > > > + > > > > 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)) { > > > > + (cmd & PCI_COMMAND_MASTER)) { > > > > + /* Bus driver disables bus mastering - make it act > > > > + * as a kind of reset to render the device quiescent. */ > > > > virtio_pci_stop_ioeventfd(proxy); > > > > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > > > > + virtio_reset(vdev); > > > > + msix_unuse_all_vectors(&proxy->pci_dev); > > > > } > > > > } > > > > > > > > @@ -895,11 +889,19 @@ 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) && > > > > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > > > > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > > + /* Linux before 2.6.34 drives the device without enabling > > > > + the PCI device bus master bit. Enable it automatically > > > > + for the guest. This is a PCI spec violation but so is > > > > + initiating DMA with bus master bit clear. > > > > + Note: this only makes a difference when migrating > > > > + across QEMU versions from an old QEMU, as for new QEMU > > > > + bus master and driver bits are always in sync. > > > > + TODO: consider enabling conditionally for compat machine types. */ > > > > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > > > > + VIRTIO_CONFIG_S_DRIVER)) { > > > > + 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,7 +1042,6 @@ 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[] = { > > It's a race condition: you have to catch it migrating in the window before DRIVER_OK is set. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-06 16:26 ` Michael S. Tsirkin @ 2014-10-06 16:46 ` Greg Kurz 2014-10-06 17:25 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2014-10-06 16:46 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On Mon, 6 Oct 2014 19:26:21 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Oct 06, 2014 at 04:51:35PM +0200, Greg Kurz wrote: > > On Wed, 17 Sep 2014 20:39:25 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Sep 17, 2014 at 07:21:09PM +0200, Greg Kurz wrote: > > > > On Sun, 14 Sep 2014 21:30:36 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > Current support for bus master (clearing OK bit) > > > > > together with the need to support guests which do not > > > > > enable PCI bus mastering, leads to extra state in > > > > > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > > > > > in case of cross-version migration for the case when > > > > > guests use the device before setting DRIVER_OK. > > > > > > > > > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > > > > > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > > > > > guests never touch this bit so they will work. > > > > > > > > > > As reset clears device status, DRIVER and MASTER bits are > > > > > now in sync, so we can fix up cross-version migration simply > > > > > by synchronising them, without need to detect a buggy guest > > > > > explicitly. > > > > > > > > > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > > > > > > > > > As reset makes the device quiescent, in the future we'll be able to drop > > > > > checking OK bit in a bunch of places. > > > > > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > > > > > Hi Michael, > > > > > > > > I am not quite sure how to test this patch with my pseries based setup... > > > > Migrating from qemu-2.1 to qemu-master ? > > > > > > > > Cheers. > > > > > > > > -- > > > > Greg > > > > > > Exactly. And back! Pls don't forget to specify the 2.1 machine type. > > > Thanks! > > > > > > > Michael, > > > > Nikunj and I had started to investigate the pseries breakage: the QEMU > > originated reset brought by this patch clears the vq and breaks SLOF. > > This isn't a surprise since reset should always come from the driver, > > not the device. > > > > Since commit 45363e46aeebfc99753389649eac7c7fc22bfe52 has reverted this > > patch, QEMU works again for pseries and virtio. :) > > > > So back to the initial issue, I've tried to migrate a pseries-2.1 guest running > > rhel65, from QEMU v2.1.2 to QEMU master, back and forth, several times and it > > always succeeded... what symptom this patch was expected to fix ? > > > > Cheers. > > > > -- > > Greg > > > > > > > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > > > > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > > > index a827cd4..f560814 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); > > > > > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > > > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > > > > > > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > > > > > + > > > > > 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)) { > > > > > + (cmd & PCI_COMMAND_MASTER)) { > > > > > + /* Bus driver disables bus mastering - make it act > > > > > + * as a kind of reset to render the device quiescent. */ > > > > > virtio_pci_stop_ioeventfd(proxy); > > > > > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > > > > > + virtio_reset(vdev); > > > > > + msix_unuse_all_vectors(&proxy->pci_dev); > > > > > } > > > > > } > > > > > > > > > > @@ -895,11 +889,19 @@ 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) && > > > > > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > > > > > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > > > + /* Linux before 2.6.34 drives the device without enabling > > > > > + the PCI device bus master bit. Enable it automatically > > > > > + for the guest. This is a PCI spec violation but so is > > > > > + initiating DMA with bus master bit clear. > > > > > + Note: this only makes a difference when migrating > > > > > + across QEMU versions from an old QEMU, as for new QEMU > > > > > + bus master and driver bits are always in sync. > > > > > + TODO: consider enabling conditionally for compat machine types. */ > > > > > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > > > > > + VIRTIO_CONFIG_S_DRIVER)) { > > > > > + 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,7 +1042,6 @@ 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[] = { > > > > > > It's a race condition: you have to catch it migrating > in the window before DRIVER_OK is set. > Ok I see... when DRIVER and MASTER bits are not in sync. I'll patch QEMU to trigger a migration in this window then. Thanks. -- Greg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-06 16:46 ` Greg Kurz @ 2014-10-06 17:25 ` Michael S. Tsirkin 2014-10-13 8:49 ` Greg Kurz 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2014-10-06 17:25 UTC (permalink / raw) To: Greg Kurz; +Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On Mon, Oct 06, 2014 at 06:46:17PM +0200, Greg Kurz wrote: > On Mon, 6 Oct 2014 19:26:21 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Mon, Oct 06, 2014 at 04:51:35PM +0200, Greg Kurz wrote: > > > On Wed, 17 Sep 2014 20:39:25 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Sep 17, 2014 at 07:21:09PM +0200, Greg Kurz wrote: > > > > > On Sun, 14 Sep 2014 21:30:36 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > Current support for bus master (clearing OK bit) > > > > > > together with the need to support guests which do not > > > > > > enable PCI bus mastering, leads to extra state in > > > > > > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > > > > > > in case of cross-version migration for the case when > > > > > > guests use the device before setting DRIVER_OK. > > > > > > > > > > > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > > > > > > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > > > > > > guests never touch this bit so they will work. > > > > > > > > > > > > As reset clears device status, DRIVER and MASTER bits are > > > > > > now in sync, so we can fix up cross-version migration simply > > > > > > by synchronising them, without need to detect a buggy guest > > > > > > explicitly. > > > > > > > > > > > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > > > > > > > > > > > As reset makes the device quiescent, in the future we'll be able to drop > > > > > > checking OK bit in a bunch of places. > > > > > > > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > --- > > > > > > > > > > Hi Michael, > > > > > > > > > > I am not quite sure how to test this patch with my pseries based setup... > > > > > Migrating from qemu-2.1 to qemu-master ? > > > > > > > > > > Cheers. > > > > > > > > > > -- > > > > > Greg > > > > > > > > Exactly. And back! Pls don't forget to specify the 2.1 machine type. > > > > Thanks! > > > > > > > > > > Michael, > > > > > > Nikunj and I had started to investigate the pseries breakage: the QEMU > > > originated reset brought by this patch clears the vq and breaks SLOF. > > > This isn't a surprise since reset should always come from the driver, > > > not the device. > > > > > > Since commit 45363e46aeebfc99753389649eac7c7fc22bfe52 has reverted this > > > patch, QEMU works again for pseries and virtio. :) > > > > > > So back to the initial issue, I've tried to migrate a pseries-2.1 guest running > > > rhel65, from QEMU v2.1.2 to QEMU master, back and forth, several times and it > > > always succeeded... what symptom this patch was expected to fix ? > > > > > > Cheers. > > > > > > -- > > > Greg > > > > > > > > > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > > > > > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > > > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > > > > index a827cd4..f560814 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); > > > > > > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > > > > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > > > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > > > > > > > > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > > > > > > + > > > > > > 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)) { > > > > > > + (cmd & PCI_COMMAND_MASTER)) { > > > > > > + /* Bus driver disables bus mastering - make it act > > > > > > + * as a kind of reset to render the device quiescent. */ > > > > > > virtio_pci_stop_ioeventfd(proxy); > > > > > > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > > > > > > + virtio_reset(vdev); > > > > > > + msix_unuse_all_vectors(&proxy->pci_dev); > > > > > > } > > > > > > } > > > > > > > > > > > > @@ -895,11 +889,19 @@ 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) && > > > > > > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > > > > > > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > > > > + /* Linux before 2.6.34 drives the device without enabling > > > > > > + the PCI device bus master bit. Enable it automatically > > > > > > + for the guest. This is a PCI spec violation but so is > > > > > > + initiating DMA with bus master bit clear. > > > > > > + Note: this only makes a difference when migrating > > > > > > + across QEMU versions from an old QEMU, as for new QEMU > > > > > > + bus master and driver bits are always in sync. > > > > > > + TODO: consider enabling conditionally for compat machine types. */ > > > > > > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > > > > > > + VIRTIO_CONFIG_S_DRIVER)) { > > > > > > + 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,7 +1042,6 @@ 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[] = { > > > > > > > > > > It's a race condition: you have to catch it migrating > > in the window before DRIVER_OK is set. > > > > Ok I see... when DRIVER and MASTER bits are not in sync. I'll patch QEMU to > trigger a migration in this window then. > > Thanks. BTW I reverted that patch, and to fix migration, I'm thinking about applying the following patch on top of master. Would appreciate testing cross-version migration (2.1 to master) with this patch applied. 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 \ 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(), }; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-06 17:25 ` Michael S. Tsirkin @ 2014-10-13 8:49 ` Greg Kurz 2014-10-13 9:01 ` Michael S. Tsirkin 2014-10-13 11:09 ` Michael S. Tsirkin 0 siblings, 2 replies; 14+ messages in thread From: Greg Kurz @ 2014-10-13 8:49 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On Mon, 6 Oct 2014 20:25:04 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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(), > }; > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-13 8:49 ` Greg Kurz @ 2014-10-13 9:01 ` Michael S. Tsirkin 2014-10-13 10:42 ` Greg Kurz 2014-10-13 11:09 ` Michael S. Tsirkin 1 sibling, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2014-10-13 9:01 UTC (permalink / raw) To: Greg Kurz; +Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori 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" <mst@redhat.com> 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? > > 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(), > > }; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-13 9:01 ` Michael S. Tsirkin @ 2014-10-13 10:42 ` Greg Kurz 2014-10-13 12:29 ` Alexander Graf 0 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2014-10-13 10:42 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Nikunj A Dadhania, Alexey Kardashevskiy, Jason Wang, qemu-devel, Alexander Graf, Anthony Liguori On Mon, 13 Oct 2014 12:01:07 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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? > (Cc'ing Alexander and Alexey) It is still at an early stage, but we have that: commit 6026db4501f773caaa2895cde7f93022960c7169 Author: Alexey Kardashevskiy <aik@ozlabs.ru> Date: Wed Jun 25 14:08:45 2014 +1000 spapr: Define a 2.1 pseries machine > > > 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(), > > > }; > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-13 10:42 ` Greg Kurz @ 2014-10-13 12:29 ` Alexander Graf 2014-10-13 12:40 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Alexander Graf @ 2014-10-13 12:29 UTC (permalink / raw) To: Greg Kurz, Michael S. Tsirkin Cc: Alexey Kardashevskiy, Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On 13.10.14 12:42, Greg Kurz wrote: > On Mon, 13 Oct 2014 12:01:07 +0300 > "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-13 12:29 ` Alexander Graf @ 2014-10-13 12:40 ` Michael S. Tsirkin 0 siblings, 0 replies; 14+ messages in thread From: Michael S. Tsirkin @ 2014-10-13 12:40 UTC (permalink / raw) To: Alexander Graf Cc: Nikunj A Dadhania, Alexey Kardashevskiy, Jason Wang, qemu-devel, Anthony Liguori 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" <mst@redhat.com> 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" <mst@redhat.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-13 8:49 ` Greg Kurz 2014-10-13 9:01 ` Michael S. Tsirkin @ 2014-10-13 11:09 ` Michael S. Tsirkin 2014-10-13 16:07 ` Greg Kurz 1 sibling, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2014-10-13 11:09 UTC (permalink / raw) To: Greg Kurz; +Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori 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" <mst@redhat.com> 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. > 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(), > > }; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master 2014-10-13 11:09 ` Michael S. Tsirkin @ 2014-10-13 16:07 ` Greg Kurz 0 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2014-10-13 16:07 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Nikunj A Dadhania, qemu-devel, Anthony Liguori On Mon, 13 Oct 2014 14:09:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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(), > > > }; > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-10-13 16:08 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-14 18:30 [Qemu-devel] [PATCH] virtio-pci: fix migration for pci bus master Michael S. Tsirkin 2014-09-17 17:21 ` Greg Kurz 2014-09-17 17:39 ` Michael S. Tsirkin 2014-10-06 14:51 ` Greg Kurz 2014-10-06 16:26 ` Michael S. Tsirkin 2014-10-06 16:46 ` Greg Kurz 2014-10-06 17:25 ` Michael S. Tsirkin 2014-10-13 8:49 ` Greg Kurz 2014-10-13 9:01 ` Michael S. Tsirkin 2014-10-13 10:42 ` Greg Kurz 2014-10-13 12:29 ` Alexander Graf 2014-10-13 12:40 ` Michael S. Tsirkin 2014-10-13 11:09 ` Michael S. Tsirkin 2014-10-13 16:07 ` Greg Kurz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).