diff mbox

virtio-pci: enable bus master for old guests

Message ID 20140911164533.24109.16197.stgit@bahia.local
State New
Headers show

Commit Message

Greg Kurz Sept. 11, 2014, 4:45 p.m. UTC 
From: Michael S. Tsirkin <mst@redhat.com>

commit cc943c36faa192cd4b32af8fe5edb31894017d35
    pci: Use bus master address space for delivering MSI/MSI-X messages
breaks virtio-net for rhel6.[56] x86 guests because they don't
enable bus mastering for virtio PCI devices. For the same reason,
rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.

Old guests forgot to enable bus mastering, enable it automatically on
DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
these old guests to disable bus mastering (this happens when the driver
probes the device). And we must also re-enable bus mastering after migration.

Cc: qemu-stable@nongnu.org
Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
[ old guest detection on DRIVER,
  squashed patch from Michael S. Tsirkin to re-enable bus mastering,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Tested-by: Cedric Le Goater <clg@fr.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-pci.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Michael S. Tsirkin Sept. 11, 2014, 5:47 p.m. UTC | #1 
On Thu, Sep 11, 2014 at 06:45:33PM +0200, Greg Kurz wrote:
> From: Michael S. Tsirkin <mst@redhat.com>
> 
> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>     pci: Use bus master address space for delivering MSI/MSI-X messages
> breaks virtio-net for rhel6.[56] x86 guests because they don't
> enable bus mastering for virtio PCI devices. For the same reason,
> rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.
> 
> Old guests forgot to enable bus mastering, enable it automatically on
> DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
> these old guests to disable bus mastering (this happens when the driver
> probes the device). And we must also re-enable bus mastering after migration.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> [ old guest detection on DRIVER,
>   squashed patch from Michael S. Tsirkin to re-enable bus mastering,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Tested-by: Cedric Le Goater <clg@fr.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

I think this is too fragile. Let's try to figure out a strategy
that does not depend on VIRTIO_PCI_FLAG_BUS_MASTER_BUG.

> ---
>  hw/virtio/virtio-pci.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..f981841 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -317,9 +317,11 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          /* 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) &&
> +        if ((val & VIRTIO_CONFIG_S_DRIVER) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
> @@ -473,10 +475,14 @@ 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)) {
> -        virtio_pci_stop_ioeventfd(proxy);
> -        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> +        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
> +        } else {
> +            virtio_pci_stop_ioeventfd(proxy);
> +            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> +        }
>      }
>  }
>  
> @@ -890,6 +896,8 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running)
>          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;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          virtio_pci_start_ioeventfd(proxy);
>      } else {
Michael S. Tsirkin Sept. 11, 2014, 5:50 p.m. UTC | #2 
On Thu, Sep 11, 2014 at 08:47:01PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 11, 2014 at 06:45:33PM +0200, Greg Kurz wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > 
> > commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >     pci: Use bus master address space for delivering MSI/MSI-X messages
> > breaks virtio-net for rhel6.[56] x86 guests because they don't
> > enable bus mastering for virtio PCI devices. For the same reason,
> > rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.
> > 
> > Old guests forgot to enable bus mastering, enable it automatically on
> > DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
> > these old guests to disable bus mastering (this happens when the driver
> > probes the device). And we must also re-enable bus mastering after migration.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > [ old guest detection on DRIVER,
> >   squashed patch from Michael S. Tsirkin to re-enable bus mastering,
> >   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Tested-by: Cedric Le Goater <clg@fr.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> I think this is too fragile. Let's try to figure out a strategy
> that does not depend on VIRTIO_PCI_FLAG_BUS_MASTER_BUG.

OTOH it does work, so I think I'll apply something similar (this one is
slightly broken wrt migration) and work on simplifications as a follow-up.

> > ---
> >  hw/virtio/virtio-pci.c |   18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..f981841 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -317,9 +317,11 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          /* 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) &&
> > +        if ((val & VIRTIO_CONFIG_S_DRIVER) &&
> >              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          break;
> >      case VIRTIO_MSI_CONFIG_VECTOR:
> > @@ -473,10 +475,14 @@ 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)) {
> > -        virtio_pci_stop_ioeventfd(proxy);
> > -        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> > +        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > +        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> > +        } else {
> > +            virtio_pci_stop_ioeventfd(proxy);
> > +            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> > +        }
> >      }
> >  }
> >  
> > @@ -890,6 +896,8 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running)
> >          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;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          virtio_pci_start_ioeventfd(proxy);
> >      } else {
diff mbox

Patch

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ddb5da1..f981841 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -317,9 +317,11 @@  static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         /* 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) &&
+        if ((val & VIRTIO_CONFIG_S_DRIVER) &&
             !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
             proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
         }
         break;
     case VIRTIO_MSI_CONFIG_VECTOR:
@@ -473,10 +475,14 @@  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)) {
-        virtio_pci_stop_ioeventfd(proxy);
-        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
+        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
+        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
+        } else {
+            virtio_pci_stop_ioeventfd(proxy);
+            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
+        }
     }
 }
 
@@ -890,6 +896,8 @@  static void virtio_pci_vmstate_change(DeviceState *d, bool running)
         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;
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
         }
         virtio_pci_start_ioeventfd(proxy);
     } else {